[libvirt] [PATCH] conf: Format numatune XML correctly while placement is none

[Daniel P. Berrange]

On Wed, Jun 20, 2012 at 05:09:38PM +0800, Osier Yang wrote:
> setNumaParameters tunes the numa setting using cgroup, it's another
> entry except libnuma/numad for numa tuning. And it doesn't set the
> placement, and further more, the formating codes doesn't take this
> into consideration.
> 
> How to reproduce:
> 
> conn = libvirt.open(None)
> dom = conn.lookupByName('linux')
> param = {'numa_nodeset': '0', 'numa_mode': 1}
> dom.setNumaParameters(param, 2)
> 
> % virsh start linux
> error: Failed to start domain rhel6.3rc
> error: (domain_definition):8: error parsing attribute name
>     <memory mode='preferred'   </numatune>
> -------------------------------^
> 
> ---
> By the way, I see problems of setNumaParameters too.
> 
> conn = libvirt.open(None)
> dom = conn.lookupByName('linux')
> param = {'numa_mode': 1}
> dom.setNumaParameters(param, 2)
> 
> The numa 'mode' will be just ignored, and no 'numatune' XML is formated,
> as neither 'nodeset' nor 'placement' is specified. I'd think it's
> right to ignore it when formating, it's meaningless to only specify
> the 'mode'. However, we might have to fix setNumaParameters to prevent
> setting the numa mode without nodeset, and error out, as it's really a
> bad user experience to see the API call succeeded, but the expected
> XML doesn't show up in the end.
> ---
>  src/conf/domain_conf.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 81c6308..c44d89d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12795,23 +12795,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          const char *placement;
>  
>          mode = virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode);
> -        virBufferAsprintf(buf, "    <memory mode='%s' ", mode);
> +        virBufferAsprintf(buf, "    <memory mode='%s'", mode);
>  
> -        if (def->numatune.memory.placement_mode ==
> -            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
> +        if (def->numatune.memory.nodemask) {
>              nodemask = virDomainCpuSetFormat(def->numatune.memory.nodemask,
> -                                         VIR_DOMAIN_CPUMASK_LEN);
> +                                             VIR_DOMAIN_CPUMASK_LEN);
>              if (nodemask == NULL) {
>                  virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                       _("failed to format nodeset for "
>                                         "NUMA memory tuning"));
>                  goto cleanup;
>              }
> -            virBufferAsprintf(buf, "nodeset='%s'/>\n", nodemask);
> +            virBufferAsprintf(buf, " nodeset='%s'/>\n", nodemask);
>              VIR_FREE(nodemask);
> -        } else if (def->numatune.memory.placement_mode) {
> +        } else if (def->numatune.memory.placement_mode ==
> +            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
>              placement = virDomainNumatuneMemPlacementModeTypeToString(def->numatune.memory.placement_mode);
> -            virBufferAsprintf(buf, "placement='%s'/>\n", placement);
> +            virBufferAsprintf(buf, " placement='%s'/>\n", placement);
> +        } else {
> +            /* Should not hit here. */
> +            virBufferAddLit(buf, "/>\n");
>          }
>          virBufferAddLit(buf, "  </numatune>\n");
>      }

The fact that we had such a horrific XML formatting bug shows that
there is a gap in our test coverage. Please add additional test
cases to cover this scenario


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|