[libvirt] [PATCH 29/34] conf: Don't store vcpusched orthogonally to other vcpu info

[John Ferlan]

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;
> +}
> +
> +

[...]