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

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

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

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