Re: [libvirt] [PATCH v4 2/2 RESEND] qemu: Add command line parser for NVRAM.

On 18/04/13 13:40, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > This patch is to add command line parser for NVRAM device, s/parser/builder and parser/, I didn't go through it carefully, but what I catched with a rough look...

> and add test cases. > > Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_command.c | 72 ++++++++++++++++++++++ > src/qemu/qemu_command.h | 2 + > tests/qemuargv2xmltest.c | 2 + > .../qemuxml2argv-pseries-nvram.args | 1 + > .../qemuxml2argv-pseries-nvram.xml | 22 +++++++ > tests/qemuxml2argvtest.c | 1 + > 6 files changed, 100 insertions(+) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml Don't we need a new capability flag to detect if the device is supported by qemu? or it's already supported?

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index d899239..490881e 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1172,6 +1172,15 @@ int > qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, > goto cleanup; > } > + if (def->nvram) { > + if (def->os.arch == VIR_ARCH_PPC64 && > + STREQ(def->os.machine, "pseries")) > + def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; > + if (qemuAssignSpaprVIOAddress(def, &def->nvram->info, > + 0x3000ul) < 0) What does "0x3000ul" means, should we have a macro for it?

> + goto cleanup; > + } > + > /* No other devices are currently supported on spapr-vio */ > ret = 0; > @@ -3960,6 +3969,32 @@ error: > return NULL; > } > +char * Is it used externally? as far as I see, no, so it should be static.

> +qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && > + dev->info.addr.spaprvio.has_reg) { > + virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx", > + dev->info.addr.spaprvio.reg); > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + "%s", _("NVRAM device address is not supported.\n")); It should be XML error, but not "not supported".

> + goto error; > + } > + > + if (virBufferError(&buf)) { > + virReportOOMError(); > + goto error; > + } > + > + return virBufferContentAndReset(&buf); > + > +error: > + virBufferFreeAndReset(&buf); > + return NULL; > +} > char * > qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, > @@ -7662,6 +7697,23 @@ qemuBuildCommandLine(virConnectPtr conn, > goto error; > } > + if (def->nvram) { > + if (def->os.arch == VIR_ARCH_PPC64 && > + STREQ(def->os.machine, "pseries")) { > + char *optstr; > + virCommandAddArg(cmd, "-global"); > + optstr = qemuBuildNVRAMDevStr(def->nvram); > + if (!optstr) > + goto error; > + if (optstr) > + virCommandAddArg(cmd, optstr); > + VIR_FREE(optstr); > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("NVRAM device is not supported")); How about make it more clear: "nvram device is only supported for......", as this sounds like it's not supported anyway.

> + goto error; > + } > + } > if (snapshot) > virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); > @@ -9770,6 +9822,26 @@ virDomainDefPtr > qemuParseCommandLine(virCapsPtr qemuCaps, > goto error; > } > + } else if (STREQ(arg, "-global") && > + STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) { > + > + WANT_VALUE(); > + > + if (VIR_ALLOC(def->nvram) < 0) > + goto no_memory; > + > + def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; > + def->nvram->info.addr.spaprvio.has_reg = true; > + > + val += strlen("spapr-nvram.reg="); > + if (virStrToLong_ull(val, NULL, 16, > + &def->nvram->info.addr.spaprvio.reg) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, Not right error type.. Generally we report error like this: virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse drive index '%s'"), val);

> + _("invalid value for spapr-nvram.reg :" > + "'%s'"), val); > + goto error; Hope def->nvram is freed somwhere, such as virDomainDefFree.

> + } > + Useless blank line. > } else if (STREQ(arg, "-S")) { > /* ignore, always added by libvirt */ > } else { > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index 1789c20..ecd4f02 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -118,6 +118,8 @@ char * > qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, > char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, > virQEMUCapsPtr qemuCaps); > +char * qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev); And this should be removed. Per it's static helper. > + > char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, > virQEMUCapsPtr qemuCaps); > diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c > index 9167d88..1ee7aa2 100644 > --- a/tests/qemuargv2xmltest.c > +++ b/tests/qemuargv2xmltest.c > @@ -243,6 +243,8 @@ mymain(void) > DO_TEST("hyperv"); > + DO_TEST("pseries-nvram"); > + > DO_TEST_FULL("restore-v1", 0, "stdio"); > DO_TEST_FULL("restore-v2", 0, "stdio"); > DO_TEST_FULL("restore-v2", 0, "exec:cat"); > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args > b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args > new file mode 100644 > index 0000000..ca5333e > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.args > @@ -0,0 +1 @@ > +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test > /usr/bin/qemu-system-ppc64 -S -M pseries -m 512 -smp 1 -nographic > -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb > -net none -serial none -parallel none -global spapr-nvram.reg=0x4000 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml > b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml Long line, see how other cases do.

> new file mode 100644 > index 0000000..d7be30f > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml > @@ -0,0 +1,22 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> > + <memory unit='KiB'>524288</memory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='ppc64' machine='pseries'>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-system-ppc64</emulator> > + <controller type='usb' index='0'/> > + <memballoon model='virtio'/> > + <nvram> > + <address type='spapr-vio' reg='0x4000'/> > + </nvram> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index d6575e7..1a5d37f 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -904,6 +904,7 @@ mymain(void) > QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); > DO_TEST_ERROR("pseries-vio-address-clash", QEMU_CAPS_DRIVE, > QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); > + DO_TEST("pseries-nvram", NONE); > DO_TEST("disk-ide-drive-split", > QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, > QEMU_CAPS_IDE_CD); I think you will want xml2xml test too. To make sure the XML formating works as expected.

Osier