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

[Peter Krempa]

On Sat, Jan 16, 2016 at 10:23:13 -0500, John Ferlan wrote:
> 
> 
> 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(-)
> > 

[...]

> > +    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)) {

So start of this loop guarantees, that theres at least one element in
the bitmap ...

> > +            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]

... thus this won't return -1 in any case here. Coverity is obviously
wrong as usual since it's terrible at introspecting the bitmap code.

> 
> > +                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.

So this doesn't make sense.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160129/5998a92c/attachment-0001.sig>