[libvirt] [PATCHv2] numatune: setting --mode does not work well

[Ján Tomko]

On 08/20/2014 03:46 PM, Erik Skultety wrote:
> When trying to set numatune mode directly using virsh numatune command,
> correct error is raised, however numatune structure was not deallocated,
> thus resulting in creating an empty numatune element in the guest XML,
> if none was present before. Running the same command aftewards results
> in a successful change with broken XML structure. Patch fixes the
> deallocation problem as well as checking for invalid attribute
> combination VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO + a nonempty nodeset.
> 
> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1129998
> ---
>  src/conf/numatune_conf.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
> index 48d1d04..3bfac56 100644
> --- a/src/conf/numatune_conf.c
> +++ b/src/conf/numatune_conf.c
> @@ -439,7 +439,7 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>  {
>      bool create = !*numatunePtr;  /* Whether we are creating new struct */
>      int ret = -1;
> -    virDomainNumatunePtr numatune = NULL;
> +    virDomainNumatunePtr numatune = *numatunePtr;
>  
>      /* No need to do anything in this case */
>      if (mode == -1 && placement == -1 && !nodeset)
> @@ -461,9 +461,15 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>          goto cleanup;
>      }
>  
> -    if (create && VIR_ALLOC(*numatunePtr) < 0)
> +    if (placement_static && !nodeset) {

What I did not notice in review of v1:
this fails 'make check':
310) QEMU XML-2-ARGV numad-auto-memory-vcpu-cpuset                     ...
libvirt: Domain Config error : unsupported configuration: nodeset for NUMA
memory tuning must be set if 'placement' is 'static'
FAILED
311) QEMU XML-2-ARGV numad-auto-memory-vcpu-no-cpuset-and-placement    ...
libvirt: Domain Config error : unsupported configuration: nodeset for NUMA
memory tuning must be set if 'placement' is 'static'
FAILED
312) QEMU XML-2-ARGV numad-static-memory-auto-vcpu                     ...
libvirt: Domain Config error : unsupported configuration: nodeset for NUMA
memory tuning must be set if 'placement' is 'static'
FAILED

I think it would be better to leave the error where it is and free numatune in
cleanup if we just allocated it.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("nodeset for NUMA memory tuning must be set "
> +                         "if 'placement' is 'static'"));
> +        goto cleanup;
> +    }
> +
> +    if (create && VIR_ALLOC(numatune) < 0)
>          goto cleanup;
> -    numatune = *numatunePtr;
>  
>      if (create) {
>          /* Defaults for new struct */
> @@ -492,12 +498,11 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>              placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO;
>      }
>  
> -    if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC &&
> -        !numatune->memory.nodeset) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("nodeset for NUMA memory tuning must be set "
> -                         "if 'placement' is 'static'"));
> -        goto cleanup;
> +    /* setting nodeset when placement auto is invalid */
> +    if (placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO &&
> +        numatune->memory.nodeset) {
> +        virBitmapFree(numatune->memory.nodeset);
> +        numatune->memory.nodeset = NULL;
>      }
>  
>      if (placement != -1)
> @@ -505,8 +510,17 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>  
>      numatune->memory.specified = true;
>  
> +    if (create) {
> +        *numatunePtr = numatune;
> +        numatune = NULL;
> +    }
> +

>      ret = 0;
> +    return ret;

This can be written as 'return 0;'

> +
>   cleanup:

Usually, the 'cleanup' label is used for code that is run on both success and
error paths. If it's only for errors, 'error' is the preferred label.

Alternatively, leave the label as 'cleanup' and use
if (create && ret < 0)

> +    if (create)
> +        virDomainNumatuneFree(numatune);
>      return ret;
>  }
>  
> 

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140821/dfac3960/attachment-0001.sig>