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

[John Ferlan]

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