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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list