[libvirt] [PATCHv5 12/19] conf: Refactor virDomainResctrlAppend

[John Ferlan]

This is more "Introduce virDomainResctrlNew"

On 10/9/18 6:30 AM, Wang Huaqiang wrote:
> Refactor virDomainResctrlAppend to facilitate virDomainResctrlDef
> with the capability to hold more element.

and then this says something like:

"Rather than rely on virDomainResctrlAppend to perform the allocation,
move the onus to the caller and make use of virBitmapNewCopy for @vcpus
and virObjectRef for @alloc, thus removing the need to set each to NULL
after the call."

> 
> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> ---
>  src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e2b4701..9a514a6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18920,24 +18920,43 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
>  }
>  
>  
> +static virDomainResctrlDefPtr
> +virDomainResctrlNew(virResctrlAllocPtr alloc,
> +                    virBitmapPtr vcpus)
> +{
> +    virDomainResctrlDefPtr resctrl = NULL;
> +
> +    if (VIR_ALLOC(resctrl) < 0)
> +        return NULL;
> +
> +    if ((resctrl->vcpus = virBitmapNewCopy(vcpus)) == NULL) {

I'd prefer:

    if (!(resctrl->vcpus = virBitmapNewCopy(vcpus))) {

> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to copy 'vcpus'"));
> +        goto error;
> +    }
> +
> +    resctrl->alloc = virObjectRef(alloc);
> +
> +    return resctrl;
> + error:
> +    virDomainResctrlDefFree(resctrl);
> +    return NULL;
> +}
> +
> +
>  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;
>      int ret = -1;
>  
> -    if (VIR_ALLOC(tmp_resctrl) < 0)
> -        goto cleanup;
> -

Based on below, I think this is where you call the virDomainResctrlNew
w/ @cpus and @alloc

>      /* We need to format it back because we need to be consistent in the naming
>       * even when users specify some "sub-optimal" string there. */
> -    vcpus_str = virBitmapFormat(vcpus);
> +    vcpus_str = virBitmapFormat(resctrl->vcpus);
>      if (!vcpus_str)
>          goto cleanup;
>  
> @@ -18954,18 +18973,14 @@ virDomainResctrlAppend(virDomainDefPtr def,
>              goto cleanup;
>      }
>  
> -    if (virResctrlAllocSetID(alloc, alloc_id) < 0)
> +    if (virResctrlAllocSetID(resctrl->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);

You'd keep the above by use @resctrl for a parameter. On success,
VIR_APPEND_ELEMENT will set resctrl = NULL so the *DefFree won't do
anything. Without that, then the @resctrl would be leaked if the APPEND
failed for any reason.

>      VIR_FREE(alloc_id);
>      VIR_FREE(vcpus_str);
>      return ret;
> @@ -18982,6 +18997,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>      xmlNodePtr *nodes = NULL;
>      virBitmapPtr vcpus = NULL;
>      virResctrlAllocPtr alloc = NULL;
> +    virDomainResctrlDefPtr resctrl = NULL;
>      ssize_t i = 0;
>      int n;
>      int ret = -1;
> @@ -19030,15 +19046,18 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
>          goto cleanup;
>      }
>  
> -    if (virDomainResctrlAppend(def, node, alloc, vcpus, flags) < 0)
> +    resctrl = virDomainResctrlNew(alloc, vcpus);
> +    if (!resctrl)
>          goto cleanup;
>  
> -    vcpus = NULL;
> -    alloc = NULL;
> +    if (virDomainResctrlAppend(def, node, resctrl, flags) < 0)
> +        goto cleanup;
>  
> +    resctrl = NULL;

Of course this is where it gets tricky - although you could pass
&resctrl and then use (*resctrl)-> in the function - that way upon
successful return resctrl is either added or free'd already.

Alternatively, since both areas are changing to first alloc and then
append, is there any specific reason the virDomainResctrlNew has to be
outside of virDomainResctrlAppend?

I do see the future does some other virDomainResctrlMonDefParse and
virResctrlAllocIsEmpty calls before virDomainResctrlAppend - may have to
rethink all that or just go with the &resctrl logic.  Maybe I'll have a
different thought later - let's see what happens when I get there.

John


>      ret = 0;
>   cleanup:
>      ctxt->node = oldnode;
> +    virDomainResctrlDefFree(resctrl);
>      virObjectUnref(alloc);
>      virBitmapFree(vcpus);
>      VIR_FREE(nodes);
> @@ -19196,6 +19215,8 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>      xmlNodePtr *nodes = NULL;
>      virBitmapPtr vcpus = NULL;
>      virResctrlAllocPtr alloc = NULL;
> +    virDomainResctrlDefPtr resctrl = NULL;
> +
>      ssize_t i = 0;
>      int n;
>      int ret = -1;
> @@ -19240,15 +19261,20 @@ 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)
> +        resctrl = virDomainResctrlNew(alloc, vcpus);
> +        if (!resctrl)
>              goto cleanup;
> -        vcpus = NULL;
> -        alloc = NULL;
> +
> +        if (virDomainResctrlAppend(def, node, resctrl, flags) < 0)
> +            goto cleanup;
> +
> +        resctrl = NULL;
>      }
>  
>      ret = 0;
>   cleanup:
>      ctxt->node = oldnode;
> +    virDomainResctrlDefFree(resctrl);
>      virObjectUnref(alloc);
>      virBitmapFree(vcpus);
>      VIR_FREE(nodes);
>