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.
> +
> + 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...
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list