[libvirt] [PATCH] conf: Don't output <cpu> tag if it contains no information.

[Erik Skultety]

On 04/10/2015 03:09 PM, Andrea Bolognani wrote:
> The tag is already marked as optional in the schema, so no changes
> are needed there.
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1202606
> ---
> 
> Hi everyone,
> 
> this is my first contribution to libvirt so I wholeheartedly welcome
> any criticism, suggestion or recommendation :)
> 
> Cheers.
> 
>  src/conf/cpu_conf.c                                | 33 ++++++++++++++++------
>  tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml  | 23 +++++++++++++++
>  .../qemuxml2xmlout-cpu-empty.xml                   | 21 ++++++++++++++
>  tests/qemuxml2xmltest.c                            |  1 +
>  4 files changed, 69 insertions(+), 9 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> 
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index cd3882d..8f65d55 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -435,13 +435,14 @@ virCPUDefFormatBufFull(virBufferPtr buf,
>                         bool updateCPU)
>  {
>      int ret = -1;
> +    virBuffer attributeBuf = VIR_BUFFER_INITIALIZER;
>      virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
>      int indent = virBufferGetIndent(buf, false);
>  
>      if (!def)
>          return 0;
>  
> -    virBufferAddLit(buf, "<cpu");
> +    /* Format attributes */
>      if (def->type == VIR_CPU_TYPE_GUEST) {
>          const char *tmp;
>  
> @@ -451,7 +452,7 @@ virCPUDefFormatBufFull(virBufferPtr buf,
>                                 _("Unexpected CPU mode %d"), def->mode);
>                  goto cleanup;
>              }
> -            virBufferAsprintf(buf, " mode='%s'", tmp);
> +            virBufferAsprintf(&attributeBuf, " mode='%s'", tmp);
>          }
>  
>          if (def->model &&
> @@ -463,10 +464,11 @@ virCPUDefFormatBufFull(virBufferPtr buf,
>                                 def->match);
>                  goto cleanup;
>              }
> -            virBufferAsprintf(buf, " match='%s'", tmp);
> +            virBufferAsprintf(&attributeBuf, " match='%s'", tmp);
>          }
>      }
>  
> +    /* Format children */
>      virBufferAdjustIndent(&childrenBuf, indent + 2);
>      if (def->arch)
>          virBufferAsprintf(&childrenBuf, "<arch>%s</arch>\n",
> @@ -477,16 +479,29 @@ virCPUDefFormatBufFull(virBufferPtr buf,
>      if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0)
>          goto cleanup;
>  
> -    if (virBufferUse(&childrenBuf)) {
> -        virBufferAddLit(buf, ">\n");
> -        virBufferAddBuffer(buf, &childrenBuf);
> -        virBufferAddLit(buf, "</cpu>\n");
> -    } else {
> -        virBufferAddLit(buf, "/>\n");
> +    /* Put it all together */
> +    if (virBufferUse(&attributeBuf) || virBufferUse(&childrenBuf)) {
> +
> +        /* Opening tag */
Just some minor nitpicks:
Although I love the idea of commenting the code to make it as much
understandable as possible :), I think this one ^^ comments the obvious...
> +        virBufferAddLit(buf, "<cpu");
> +
> +        /* Attributes (if any) */
same here ^^...
> +        if (virBufferUse(&attributeBuf))
> +            virBufferAddBuffer(buf, &attributeBuf);
> +
> +        /* Children (if any) and closing tag */
same here ^^
> +        if (virBufferUse(&childrenBuf)) {
> +            virBufferAddLit(buf, ">\n");
> +            virBufferAddBuffer(buf, &childrenBuf);
> +            virBufferAddLit(buf, "</cpu>\n");
> +        } else {
> +            virBufferAddLit(buf, "/>\n");
> +        }
>      }
>  
>      ret = 0;
>   cleanup:
> +    virBufferFreeAndReset(&attributeBuf);
>      virBufferFreeAndReset(&childrenBuf);
>      return ret;
>  }
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml b/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> new file mode 100644
> index 0000000..2a79826
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-empty.xml
> @@ -0,0 +1,23 @@
> +<domain type='kvm'>
> +  <name>cpu-empty</name>
> +  <uuid>1aed4c39-ad6e-4a78-9264-4ce996290d17</uuid>
> +  <memory unit='KiB'>4000768</memory>
> +  <currentMemory unit='KiB'>1048576</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <cpu>
> +  </cpu>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-kvm</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> new file mode 100644
> index 0000000..e678607
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-empty.xml
> @@ -0,0 +1,21 @@
> +<domain type='kvm'>
> +  <name>cpu-empty</name>
> +  <uuid>1aed4c39-ad6e-4a78-9264-4ce996290d17</uuid>
> +  <memory unit='KiB'>4000768</memory>
> +  <currentMemory unit='KiB'>1048576</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-kvm</emulator>
> +    <controller type='usb' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <memballoon model='virtio'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 817e408..cd0b280 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -361,6 +361,7 @@ mymain(void)
>  
>      DO_TEST("clock-utc");
>      DO_TEST("clock-localtime");
> +    DO_TEST_DIFFERENT("cpu-empty");
>      DO_TEST("cpu-kvmclock");
>      DO_TEST("cpu-host-kvmclock");
>      DO_TEST("cpu-host-passthrough-features");
> 
Other than that, it looks good, so I removed the comments marked above
and pushed. Congratulations to your first patch in libvirt ;).
Erik