[libvirt] [PATCH 07/10] conf: refactor virDomainResctrlAppend

[John Ferlan]

On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
> Changed the interface from
> virDomainResctrlAppend(virDomainDefPtr def,
>                        xmlNodePtr node,
>                        virResctrlAllocPtr alloc,
>                        virBitmapPtr vcpus,
>                        unsigned int flags);
> to
> virDomainResctrlAppend(virDomainDefPtr def,
>                         xmlNodePtr node,
>                         virDomainResctrlDefPtr resctrl,
>                         unsigned int flags);
> 
> Changes will let virDomainRestrlAppend pass through more information
> with virDomainResctrlDefPtr, such as monitoring groups associated
> with the allocation.
> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>  src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bde9fef..9a65655 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19247,17 +19247,21 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>  static int
>  virDomainResctrlAppend(virDomainDefPtr def,
>                         xmlNodePtr node,
> -                       virResctrlAllocPtr alloc,
> -                       virBitmapPtr vcpus,
> +                       virDomainResctrlDefPtr resctrl,
>                         unsigned int flags)
>  {
>      char *vcpus_str = NULL;
>      char *alloc_id = NULL;
> -    virDomainResctrlDefPtr tmp_resctrl = NULL;
> +    virResctrlAllocPtr alloc = NULL;
> +    virBitmapPtr vcpus = NULL;

No need for locals here - just change to resctrl->{alloc|vcpus}

> +
>      int ret = -1;
>  
> -    if (VIR_ALLOC(tmp_resctrl) < 0)
> -        goto cleanup;
> +    if (!resctrl)
> +        return -1;

Again, here we have a programming error without an error message which
results in a generic libvirt an error occurred. Either create a specific
error message or "assume" that your caller has done the right thing.

> +
> +    alloc = virObjectRef(resctrl->alloc);

Yikes, how many Ref's are we taking on this? [1]

I don't think this is necessary since we Unref later and currently both
callers do the Ref

> +    vcpus = resctrl->vcpus;
>  
>      /* We need to format it back because we need to be consistent in the naming
>       * even when users specify some "sub-optimal" string there. */
> @@ -19281,15 +19285,12 @@ virDomainResctrlAppend(virDomainDefPtr def,
>      if (virResctrlAllocSetID(alloc, alloc_id) < 0)
>          goto cleanup;
>  
> -    tmp_resctrl->vcpus = vcpus;
> -    tmp_resctrl->alloc = alloc;
> -
> -    if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, tmp_resctrl) < 0)
> +    if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, resctrl) < 0)
>          goto cleanup;
>  
>      ret = 0;
>   cleanup:
> -    virDomainResctrlDefFree(tmp_resctrl);
> +    virObjectUnref(alloc);
>      VIR_FREE(alloc_id);
>      VIR_FREE(vcpus_str);
>      return ret;
> @@ -19306,6 +19307,8 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>      xmlNodePtr *nodes = NULL;
>      virBitmapPtr vcpus = NULL;
>      virResctrlAllocPtr alloc = NULL;
> +    virDomainResctrlDefPtr tmp_resctrl = NULL;
> +
>      ssize_t i = 0;
>      int n;
>      int ret = -1;
> @@ -19349,15 +19352,24 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>          goto cleanup;
>      }
>  
> -    if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
> +    if (VIR_ALLOC(tmp_resctrl) < 0)
>          goto cleanup;
> -    vcpus = NULL;
> +
> +    tmp_resctrl->vcpus = vcpus;
> +    tmp_resctrl->alloc = virObjectRef(alloc);

[1] Seems the called function also takes a Ref?

> +
> +    if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
> +        goto cleanup;
> +
>      alloc = NULL;
> +    vcpus = NULL;
> +    tmp_resctrl = NULL;

This sequence is quite familiar with [2] ...

>  
>      ret = 0;
>   cleanup:
>      ctxt->node = oldnode;
>      virObjectUnref(alloc);
> +    VIR_FREE(tmp_resctrl);
>      virBitmapFree(vcpus);
>      VIR_FREE(nodes);
>      return ret;
> @@ -19514,6 +19526,7 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>      xmlNodePtr *nodes = NULL;
>      virBitmapPtr vcpus = NULL;
>      virResctrlAllocPtr alloc = NULL;
> +    virDomainResctrlDefPtr tmp_resctrl = NULL;
>      ssize_t i = 0;
>      int n;
>      int ret = -1;
> @@ -19560,17 +19573,24 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>       * just update the existing alloc information, which is done in above
>       * virDomainMemorytuneDefParseMemory */
>      if (new_alloc) {
> -        if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
> +        if (VIR_ALLOC(tmp_resctrl) < 0)
> +            goto cleanup;
> +
> +        tmp_resctrl->alloc = virObjectRef(alloc);

[1] Seems the called function also takes a Ref?

> +        tmp_resctrl->vcpus = vcpus;
> +        if (virDomainResctrlAppend(def, node, tmp_resctrl, flags) < 0)
>              goto cleanup;
>          vcpus = NULL;
>          alloc = NULL;
> +        tmp_resctrl = NULL;

[2] ... this sequence

It seems to me you could create helper :

   virDomainResctrlCreate(def, node, alloc, vcpus, flags)

which could :

    if (VIR_ALLOC(resctrl) < 0)
        return -1;

    resctrl->alloc = alloc;
    resctrl->vcpus = vcpus;
    if (virDomainResctrlAppend(def, node, resctrl, flags) < 0) {
        VIR_FREE(resctrl);
        return -1;
    }

    virObjectRef(alloc);
    return 0;

with the current callers just changing from Append to Create keeping
their alloc = NULL and vcpus = NULL on success.

John

>      }
>  
>      ret = 0;
>   cleanup:
>      ctxt->node = oldnode;
> -    virObjectUnref(alloc);
>      virBitmapFree(vcpus);
> +    virObjectUnref(alloc);
> +    VIR_FREE(tmp_resctrl);
>      VIR_FREE(nodes);
>      return ret;
>  }
>