Re: [libvirt] [PATCH v2 1/1][RESEND] Add NVRAM device

On Thu, Mar 14, 2013 at 02:54:19PM +0800, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > For pSeries guest in QEMU, NVRAM is one kind of spapr-vio device. > Users are allowed to specify spapr-vio devices'address. > But NVRAM is not supported in libvirt. So this patch is to > add NVRAM device to allow users to specify its address. > > In QEMU, NVRAM device's address is specified by > "-global spapr-nvram.reg=xxxxx". > > In libvirt, XML file is defined as the following: > > <nvram> > <address type='spapr-vio' reg='0x3000'/> > </nvram> > > Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > --- > * v2 -> v1: > Add NVRAM parameters parsing in qemuParseCommandLine. > > src/conf/domain_conf.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++- > src/conf/domain_conf.h | 10 ++++++ > src/qemu/qemu_command.c | 48 +++++++++++++++++++++++++++ > src/qemu/qemu_command.h | 2 ++ > 4 files changed, 142 insertions(+), 1 deletion(-) When adding new XML, you also need to update docs/schemas/domaincommon.rng and docs/formatdomain.html.in In addition for anything which extends the QEMU command line generator you should be adding test case(s) to tests/qemuxml2argvtest.c and also tests/qemuargv2xmltest.c

> @@ -10572,6 +10608,32 @@ virDomainDefParseXML(virCapsPtr caps, > } > } > > + def->nvram = NULL; > + if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) { > + goto error; > + } > + > + if (n > 1) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("only a single nvram device is supported")); > + goto error; > + } > + > + if (n > 0) { > + virDomainNVRAMDefPtr nvram = > + virDomainNVRAMDefParseXML(nodes[0], flags); > + if (!nvram) > + goto error; > + def->nvram = nvram; > + VIR_FREE(nodes); > + } else { > + virDomainNVRAMDefPtr nvram; > + if (VIR_ALLOC(nvram) < 0) > + goto no_memory; > + nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; > + def->nvram = nvram; This adds a <nvram> device to every single guest whether it wants one or not, which is wrong. Just delete this entire 'else' block.

NB if you had run 'make check' you would see this flaw. > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index dee493f..30694d6 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -941,6 +941,13 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, > goto cleanup; > } > > + 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) > + goto cleanup; Bad indentation level on the second 'if'

> +char * > +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); > + You should have an 'else' clause here to report an error in that scenario You must also check virBufferError() and raise an OOM error if required.

> + return virBufferContentAndReset(&buf); > +} > > char * > qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, > @@ -7006,6 +7024,19 @@ qemuBuildCommandLine(virConnectPtr conn, > } > } > > + if (def->nvram && > + 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); > + } You must have an else clause here and report VIR_ERR_CONFIG_UNSUPPORTED

> + > if (snapshot) > virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); > > @@ -9139,6 +9170,23 @@ 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; > + > + val += strlen("spapr-nvram.reg="); > + if (virStrToLong_ull(val, NULL, 16, > + &def->nvram->info.addr.spaprvio.reg) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("invalid value for spapr-nvram.reg :" > + "'%s'"), val); > + goto error; > + } > + > } else if (STREQ(arg, "-S")) { > /* ignore, always added by libvirt */ > } else { Regards, Daniel