[...]
/* Info-related functions */ int virResctrlGetInfo(virResctrlInfoPtr *resctrl) @@ -318,3 +465,866 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl, VIR_FREE(*controls); goto cleanup; } + + +/* Alloc-related functions */ +static virResctrlAllocPerTypePtr +virResctrlAllocFindType(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + bool alloc) +{
I don't like this implementation, it's too complex and it does two different things based on a bool attribute. I see the benefit that it's convenient but IMHO it's ugly.
Well I stared at it a *long* time before coming to the conclusion that as odd looking as it is - it does what it's supposed to do in a unique way compared to other libvirt functions. While yes a bit ugly, it didn't feel too complex once I got the basic premise. I also kept thinking this whole sequence relies on the "original caller" (e.g. where &resctrl originally gets passed) to be sure on failure to Unref - tracing back to that was a challenge. Thinking about these functions being called in the middle of some other code - I dunno. Still Like I pointed out - it would help *a lot* to document WTF is going on! Returning the "address of" some location based on array entry/offset does cause some concern - I was able to eventually convince myself it works... Although I must say I studied virResctrlAllocUpdateMask for quite a bit trying to determine whether what it claimed to do was actually being done. John
The only call path that doesn't need allocation is from virResctrlAllocCheckCollision(). The remaining two calls virResctrlAllocUpdateMask() and virResctrlAllocUpdateSize() needs to allocate the internal structures of *virResctrlAllocPtr* object.
Another point is that there is no need to have this function create new *virResctrlAllocPtr* object on demand, I would prefer creating that object in advance before we even start filling all the data.
+ virResctrlAllocPerLevelPtr a_level = NULL; + virResctrlAllocPtr tmp = NULL; + + if (!*resctrl) { + if (!alloc || !(*resctrl = virResctrlAllocNew())) + return NULL; + } + + tmp = *resctrl; + + /* Per-level array */ + if (tmp->nlevels <= level) { + if (!alloc || VIR_EXPAND_N(tmp->levels, tmp->nlevels, + level - tmp->nlevels + 1) < 0) + return NULL; + } + + if (!tmp->levels[level]) { + if (!alloc || + VIR_ALLOC(tmp->levels[level]) < 0 || + VIR_ALLOC_N(tmp->levels[level]->types, VIR_CACHE_TYPE_LAST) < 0) + return NULL; + } + a_level = tmp->levels[level]; + + if (!a_level->types[type]) { + if (!alloc || VIR_ALLOC(a_level->types[type]) < 0) + return NULL; + } + + return a_level->types[type]; +} + +static virBitmapPtr * +virResctrlAllocFindMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + bool alloc) +{
The code of this function can be merged into virResctrlAllocUpdateMask() and again only allocate the structures and set the mask. Currently there is no need for "Find" function if we will need it in the future it should definitely only find the mask, not allocate it.
+ virResctrlAllocPerTypePtr a_type = virResctrlAllocFindType(resctrl, level, + type, alloc); + + if (!a_type) + return NULL; + + if (!a_type->masks) { + if (!alloc || VIR_ALLOC_N(a_type->masks, cache + 1) < 0) + return NULL; + a_type->nmasks = cache + 1; + } else if (a_type->nmasks <= cache) { + if (!alloc || VIR_EXPAND_N(a_type->masks, a_type->nmasks, + cache - a_type->nmasks + 1) < 0) + return NULL; + } + + return a_type->masks + cache; +} + +static unsigned long long * +virResctrlAllocFindSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + bool alloc) +{ + virResctrlAllocPerTypePtr a_type = virResctrlAllocFindType(resctrl, level, + type, alloc);
Same as for virResctrlAllocFindMask(). With the exception that we actually need lookup function so create a one, that will only check whether there is some size set or not.
+ + if (!a_type) + return NULL; + + if (!a_type->sizes) { + if (!alloc || VIR_ALLOC_N(a_type->sizes, cache + 1) < 0) + return NULL; + a_type->nsizes = cache + 1; + } else if (a_type->nsizes <= cache) { + if (!alloc || VIR_EXPAND_N(a_type->sizes, a_type->nsizes, + cache - a_type->nsizes + 1) < 0) + return NULL; + } + + if (!a_type->sizes[cache]) { + if (!alloc || VIR_ALLOC(a_type->sizes[cache]) < 0) + return NULL; + } + + return a_type->sizes[cache]; +} + +int +virResctrlAllocUpdateMask(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + virBitmapPtr mask) +{ + virBitmapPtr *found = virResctrlAllocFindMask(resctrl, level, type, cache, + true); + + if (!found) + return -1; + + virBitmapFree(*found); + + *found = virBitmapNew(virBitmapSize(mask)); + if (!*found) + return -1; + + return virBitmapCopy(*found, mask); +} + +int +virResctrlAllocUpdateSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size) +{ + unsigned long long *found = virResctrlAllocFindSize(resctrl, level, type, + cache, true); + + if (!found) + return -1; + + *found = size; + return 0; +} + +static bool +virResctrlAllocCheckCollision(virResctrlAllocPtr a, + unsigned int level, + virCacheType type, + unsigned int cache) +{ + /* If there is an allocation for type 'both', there can be no other + * allocation for the same cache */ + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_BOTH, cache, false)) + return true; + + if (type == VIR_CACHE_TYPE_BOTH) { + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_CODE, cache, false)) + return true; + if (virResctrlAllocFindSize(&a, level, VIR_CACHE_TYPE_DATA, cache, false)) + return true; + } + + /* And never two allocations for the same type */ + if (virResctrlAllocFindSize(&a, level, type, cache, false)) + return true; + + return false; +} + +int +virResctrlAllocSetSize(virResctrlAllocPtr *resctrl, + unsigned int level, + virCacheType type, + unsigned int cache, + unsigned long long size) +{ + if (virResctrlAllocCheckCollision(*resctrl, level, type, cache)) { + virReportError(VIR_ERR_XML_ERROR, + _("Colliding cache allocations for cache " + "level '%u' id '%u', type '%s'"), + level, cache, virCacheTypeToString(type)); + return -1; + } + + return virResctrlAllocUpdateSize(resctrl, level, type, cache, size); +} + +int +virResctrlAllocForeachSize(virResctrlAllocPtr resctrl, + virResctrlAllocForeachSizeCallback cb, + void *opaque) +{ + int ret = 0; + unsigned int level = 0; + unsigned int type = 0; + unsigned int cache = 0; + + if (!resctrl) + return 0; + + for (level = 0; level < resctrl->nlevels; level++) { + virResctrlAllocPerLevelPtr a_level = resctrl->levels[level]; + + if (!a_level) + continue; + + for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { + virResctrlAllocPerTypePtr a_type = a_level->types[type]; + + if (!a_type) + continue; + + for (cache = 0; cache < a_type->nsizes; cache++) { + unsigned long long *size = a_type->sizes[cache]; + + if (!size) + continue; + + ret = cb(level, type, cache, *size, opaque); + if (ret < 0) + return ret; + } + } + } + + return 0; +} + +int +virResctrlAllocSetID(virResctrlAllocPtr alloc, + const char *id) +{ + if (!id) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Resctrl allocation id cannot be NULL")); + return -1; + } + + return VIR_STRDUP(alloc->id, id); +}
This is how I would expect all the other public functions to look like. Simple, does one thing and there is no magic.
[...]