[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

and add test cases.

Signed-off-by: Li Zhang <zhlcindy 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?


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]