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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list