[libvirt] [PATCH 23/35] conf: Add new helpers to resolve virDomainModificationImpact to domain defs

[Peter Krempa]

On Thu, Jun 04, 2015 at 07:02:00 -0400, John Ferlan wrote:
> 
> 
> On 05/29/2015 09:33 AM, Peter Krempa wrote:
> > virDomainLiveConfigHelperMethod that is used for this job now does
> > modify the flags but still requires the callers to extract the correct
> > definition objects.
> > 
> > In addition coverity and other static analyzers are usually unhappy as
> > they don't grasp the fact that @flags are upadted according to the
> > correct def to be present.
> > 
> > To work this issue around and simplify the calling chain let's add a new
> > helper that will work only on drivers that always copy the persistent
> > def to a transient at start of a vm. This will allow to drop a few
> > arguments. The new function syntax will also fill two definition
> > pointers rather than modifying the @flags parameter.
> > ---
> >  src/conf/domain_conf.c   | 113 ++++++++++++++++++++++++++++++++++++++---------
> >  src/conf/domain_conf.h   |   8 ++++
> >  src/libvirt_private.syms |   2 +
> >  3 files changed, 102 insertions(+), 21 deletions(-)
> > 
> 
> Well Coverity is most unhappy with these changes - causes 14 new issues
> - all related...  I didn't get a chance to run your series through my
> checker because not all of the patches made it through even though they
> were in the archive (strange).
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 4a8c1b5..e8bda73 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> 
> ...
> 
> > +
> > +/**
> > + * virDomainObjGetDefs:
> > + *
> > + * @vm: domain object
> > + * @flags: for virDomainModificationImpact
> > + * @liveDef: Set to the pointer to the live definition of @vm.
> > + * @persDef: Set to the pointer to the config definition of @vm.
> > + *
> > + * Helper function to resolve @flags and retrieve correct domain pointer
> > + * objects. This function should be used only when the hypervisor driver always
> > + * creates vm->newDef once the vm is started. (qemu driver does that)
> > + *
> > + * If @liveDef or @persDef are set it implies that @flags request modification
> > + * of thereof.
> > + *
> > + * Returns 0 on success and sets @liveDef and @persDef; -1 if @flags are
> > + * inappropriate.
> > + */
> > +int
> > +virDomainObjGetDefs(virDomainObjPtr vm,
> > +                    unsigned int flags,
> > +                    virDomainDefPtr *liveDef,
> > +                    virDomainDefPtr *persDef)
> > +{
> > +    if (liveDef)
> > +        *liveDef = NULL;
> > +
> > +    if (*persDef)
> > +        *persDef = NULL;
> 
> You dereference "*persDef" here without checking and furthermore this
> causes Coverity to complain about UNINIT usage in each of the callers
> since non initialized their def to NULL.a

[1]


> 
> > +
> > +    if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> > +        return -1;
> > +
> > +    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> > +        if (liveDef)
> > +            *liveDef = vm->def;
> > +
> > +        if (persDef)
> > +            *liveDef = vm->newDef;
> 
> here you check for persDef, but adjust *liveDef - so if liveDef == NULL,
> then there's going to be a failure...
> 
> > +    } else {
> > +        if (persDef)
> > +            *persDef = vm->def;
> 
> Also persDef is checked again...
> 
> > +    }
> > +
> > +    return 0;
> >  }
> > 
> > +
> >  /*
> >   * The caller must hold a lock on the driver owning 'doms',
> >   * and must also have locked 'dom', to ensure no one else
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 34b669d..262d267 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -2545,6 +2545,14 @@ virDomainObjGetPersistentDef(virCapsPtr caps,
> >                               virDomainXMLOptionPtr xmlopt,
> >                               virDomainObjPtr domain);
> > 
> > +int virDomainObjUpdateModificationImpact(virDomainObjPtr vm,
> > +                                         unsigned int *flags);
> > +
> > +int virDomainObjGetDefs(virDomainObjPtr vm,
> > +                        unsigned int flags,
> > +                        virDomainDefPtr *liveDef,
> > +                        virDomainDefPtr *persDef);
> 
> How about a "ATTRIBUTE_NONNULL(4)" here?

Actually, the first check has an extra dereference that should not be
there. The function expects that NULL is passed here.

> 
> 
> John
> 
> I'd paste a diff here that worked for me, except that I know you prefer
> not to have inlined diffs like that in email... So I'll wait to see how
> you fix this...
> 

The change is to remove the deref at [1].

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/20150604/4444f157/attachment-0001.sig>