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

[Li Zhang]

On 2013年04月18日 17:55, Osier Yang wrote:
> On 18/04/13 13:40, Li Zhang wrote:
>> From: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>>
>> 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...
>

Thanks for review.
>> and add test cases.
>>
>> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com>
>> ---
>> 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?

No, there is no a capability for it. I can add one for it.

>>
>> 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?

This is the address of this VIO device.
We assign VIO devices from 0x1000UL~0x3000UL as the default address.

To make code clear, I will create macros for all these VIO devices.

>
>> + 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.
>

No. I will change it as static.

Thanks.
>> +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".
OK.
>
>> + 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.

OK. I will modify it.
>
>> + 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);

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

Yes, it will be freed in 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.
Ah, I didn't realize that. Will modify it.

>> 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.

OK, I will add that.

Thanks.
>
> Osier