On 01/14/2016 11:27 AM, Peter Krempa wrote:
Due to bad design the vcpu sched element is orthogonal to the way how the data belongs to the corresponding objects. Now that vcpus are a struct that allow to store other info too, let's convert the data to the sane structure.
The helpers for the conversion are made universal so that they can be reused for iothreads too.
This patch also resolves https://bugzilla.redhat.com/show_bug.cgi?id=1235180 since with the correct storage approach you can't have dangling data. --- src/conf/domain_conf.c | 231 +++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 8 +- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_process.c | 8 +- 4 files changed, 202 insertions(+), 51 deletions(-)
As noted from my review of 10/34, I'm just noting something Coverity found - will review more as I get to this later.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4f6fe9..e2dda9a 100644
[...]
static int +virDomainFormatSchedDef(virDomainDefPtr def, + virBufferPtr buf, + const char *name, + virDomainThreadSchedParamPtr (*func)(virDomainDefPtr, unsigned int), + virBitmapPtr resourceMap) +{ + virBitmapPtr schedMap = NULL; + virBitmapPtr prioMap = NULL; + virDomainThreadSchedParamPtr sched; + char *tmp = NULL; + ssize_t next; + size_t i; + int ret = -1; + + if (!(schedMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN)) || + !(prioMap = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) + goto cleanup; + + for (i = VIR_PROC_POLICY_NONE + 1; i < VIR_PROC_POLICY_LAST; i++) { + virBitmapClearAll(schedMap); + + /* find vcpus using a particular scheduler */ + next = -1; + while ((next = virBitmapNextSetBit(resourceMap, next)) > -1) { + sched = func(def, next); + + if (sched->policy == i) + ignore_value(virBitmapSetBit(schedMap, next)); + } + + /* it's necessary to discriminate priority levels for schedulers that + * have them */ + while (!virBitmapIsAllClear(schedMap)) { + virBitmapPtr currentMap = NULL; + ssize_t nextprio; + bool hasPriority = false; + int priority; + + switch ((virProcessSchedPolicy) i) { + case VIR_PROC_POLICY_NONE: + case VIR_PROC_POLICY_BATCH: + case VIR_PROC_POLICY_IDLE: + case VIR_PROC_POLICY_LAST: + currentMap = schedMap; + break; + + case VIR_PROC_POLICY_FIFO: + case VIR_PROC_POLICY_RR: + virBitmapClearAll(prioMap); + hasPriority = true; + + /* we need to find a subset of vCPUs with the given scheduler + * that share the priority */ + nextprio = virBitmapNextSetBit(schedMap, -1);
Coverity notes that virBitmapNextSetBit can return -1; however, [1]
+ sched = func(def, nextprio); + priority = sched->priority; + + ignore_value(virBitmapSetBit(prioMap, nextprio));
[1] passing a -1 'nextprio' to virBitmapSetBit as 'size_t b' cannot happen.
+ + while ((nextprio = virBitmapNextSetBit(schedMap, nextprio)) > -1) { + sched = func(def, nextprio); + if (sched->priority == priority) + ignore_value(virBitmapSetBit(prioMap, nextprio)); + } + + currentMap = prioMap; + break; + } + + /* now we have the complete group */ + if (!(tmp = virBitmapFormat(currentMap))) + goto cleanup; + + virBufferAsprintf(buf, + "<%ssched %ss='%s' scheduler='%s'", + name, name, tmp, + virProcessSchedPolicyTypeToString(i)); + VIR_FREE(tmp); + + if (hasPriority) + virBufferAsprintf(buf, " priority='%d'", priority); + + virBufferAddLit(buf, "/>\n"); + + /* subtract all vCPUs that were already found */ + virBitmapSubtract(schedMap, currentMap); + } + } + + ret = 0; + + cleanup: + virBitmapFree(schedMap); + virBitmapFree(prioMap); + return ret; +} + +
[...]