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