diff mbox

[12/12] qemu: Assign device addresses in PostParse

Message ID 10f656cb5350edd3c7e7c1aa14679f78756c86a1.1452224621.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson Jan. 8, 2016, 3:50 a.m. UTC 
In order to make this work, we need to short circuit the normal
virDomainDefPostParse ordering, and manually add stock devices
ourselves, since we need them in the XML before assigning addresses.

There's a bit of test suite churn due to extra XML output, and validation
happening at different call sites, but it all looks correct to me.

There's still quite a few manual callers of qemuDomainAssignAddresses
that could be dropped too but it would need additional testing.
---
 src/qemu/qemu_domain.c                             | 10 +++++++
 src/qemu/qemu_driver.c                             |  9 ------
 .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |  4 ++-
 tests/qemuxml2argvtest.c                           | 12 ++------
 .../qemuxml2xmlout-channel-virtio-auto.xml         |  9 +++---
 .../qemuxml2xmlout-disk-scsi-vscsi.xml             | 35 ++++++++++++++++++++++
 .../qemuxml2xmlout-panic-pseries.xml               | 30 +++++++++++++++++++
 .../qemuxml2xmlout-pseries-panic-missing.xml       |  4 +--
 .../qemuxml2xmlout-pseries-panic-no-address.xml    |  4 +--
 tests/qemuxml2xmltest.c                            | 10 +++++--
 10 files changed, 97 insertions(+), 30 deletions(-)
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml

-- 
2.5.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson Jan. 12, 2016, 11:44 p.m. UTC | #1 
On 01/08/2016 06:35 AM, Peter Krempa wrote:
> On Thu, Jan 07, 2016 at 22:50:06 -0500, Cole Robinson wrote:

>> In order to make this work, we need to short circuit the normal

>> virDomainDefPostParse ordering, and manually add stock devices

>> ourselves, since we need them in the XML before assigning addresses.

>>

>> There's a bit of test suite churn due to extra XML output, and validation

>> happening at different call sites, but it all looks correct to me.

>>

>> There's still quite a few manual callers of qemuDomainAssignAddresses

>> that could be dropped too but it would need additional testing.

>> ---

>>  src/qemu/qemu_domain.c                             | 10 +++++++

>>  src/qemu/qemu_driver.c                             |  9 ------

>>  .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml |  4 ++-

>>  tests/qemuxml2argvtest.c                           | 12 ++------

>>  .../qemuxml2xmlout-channel-virtio-auto.xml         |  9 +++---

>>  .../qemuxml2xmlout-disk-scsi-vscsi.xml             | 35 ++++++++++++++++++++++

>>  .../qemuxml2xmlout-panic-pseries.xml               | 30 +++++++++++++++++++

>>  .../qemuxml2xmlout-pseries-panic-missing.xml       |  4 +--

>>  .../qemuxml2xmlout-pseries-panic-no-address.xml    |  4 +--

>>  tests/qemuxml2xmltest.c                            | 10 +++++--

>>  10 files changed, 97 insertions(+), 30 deletions(-)

>>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml

>>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml

>>

>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c

>> index a981310..e0520ab 100644

>> --- a/src/qemu/qemu_domain.c

>> +++ b/src/qemu/qemu_domain.c

>> @@ -1249,6 +1249,16 @@ qemuDomainDefPostParse(virDomainDefPtr def,

>>      if (virSecurityManagerVerify(driver->securityManager, def) < 0)

>>          goto cleanup;

>>  

>> +    /* Device defaults are normally set after calling the driver specific

>> +       PostParse routine (this function), but we need them earlier. */

>> +    if (virDomainDefPostParseDevices(def, caps, driver->xmlopt) < 0)

>> +        goto cleanup;

> 

> NACK to this, this would create a possibly dangerous situation by

> encouraging others to call the post parse callback handlers from

> themeself.

> 


What is dangerous about this? These operations are idempotent, and if they
ever failed to be idempotent, many things would break. Consider that
DefPostParseDevices is called every time a VM is redefined, or XML is read
from disk at daemon startup. Many xml2xml tests would likely fail as well.

Now is this change in good taste? debatable :)

> If it's necessary to do two passes of post parse stuff, then please add

> a new callback pointer for it.

> 


Let's say we change things to do:

virDomainDefPostParse() {
    domainPostParseCallback1();
    virDomainDefPostParseDevices();
    virDomainDefPostParseInternal();
    domainPostParseCallback2();
}

... how do we know the generic bits don't need to be called after Callback2?
We don't really. Especially because so much of the generic bits is just about
validation checking, Callback2 may have changed some bit that the generic code
needs to validate. So then maybe we do:

virDomainDefPostParse() {
    domainPostParseCallback1();
    virDomainDefPostParseDevices();
    virDomainDefPostParseInternal();
    domainPostParseCallback2();
    virDomainDefPostParseDevices();
    virDomainDefPostParseInternal();
}

But that's basically my proposed patch, except encoded generically rather than
to the only driver that currently needs.

Granted all that is hypothetical, but the point is that there isn't any clear
line that we can say 'this is for pass #1, this is for pass #2' except on a
case by case driver basis, which suggests to me it's a confusing thing to put
in generic code. If we accept the premise that the generic PostParse functions
must be idempotent, then it seems fine to me to handle any special ordering in
just one driver PostParse callback.

sidenote: IMO we _do_ need a separate PostParse entry point though, but
strictly to implement something like virDomainDefValidate() which always runs
at the end of PostParse. All it does is check for invalid config, and doesn't
change any settings. The goal would be to move as much validation as possible
out of the current PostParse and XML parsing bits into that function, so it
can be shared by impls that populate the DomainDef directly. Things seem like
they are already moving in that direction with much of the PostParse bits
already, but the validation vs. stateful bit isn't explicit. However that's a
much bigger project, and it's completely orthogonal to this patch since it
wouldn't solve my issue.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox

Patch

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a981310..e0520ab 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1249,6 +1249,16 @@  qemuDomainDefPostParse(virDomainDefPtr def,
     if (virSecurityManagerVerify(driver->securityManager, def) < 0)
         goto cleanup;
 
+    /* Device defaults are normally set after calling the driver specific
+       PostParse routine (this function), but we need them earlier. */
+    if (virDomainDefPostParseDevices(def, caps, driver->xmlopt) < 0)
+        goto cleanup;
+    if (virDomainDefAddImplicitDevices(def) < 0)
+        goto cleanup;
+
+    if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
+        goto cleanup;
+
     ret = 0;
  cleanup:
     virObjectUnref(qemuCaps);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 459401a..4290c9e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1726,9 +1726,6 @@  static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
     if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator)))
         goto cleanup;
 
-    if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
-        goto cleanup;
-
     if (!(vm = virDomainObjListAdd(driver->domains, def,
                                    driver->xmlopt,
                                    VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
@@ -7266,9 +7263,6 @@  static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
     if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
         goto cleanup;
 
-    if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
-        goto cleanup;
-
     /* do fake auto-alloc of graphics ports, if such config is used */
     for (i = 0; i < def->ngraphics; ++i) {
         virDomainGraphicsDefPtr graphics = def->graphics[i];
@@ -7502,9 +7496,6 @@  static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml
     if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator)))
         goto cleanup;
 
-    if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
-        goto cleanup;
-
     if (!(vm = virDomainObjListAdd(driver->domains, def,
                                    driver->xmlopt,
                                    0, &oldDef)))
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
index 39f4a1f..256f8c6 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml
@@ -28,7 +28,9 @@ 
       <address type='drive' controller='0' bus='0' target='0' unit='2'/>
     </disk>
     <controller type='usb' index='0'/>
-    <controller type='scsi' index='0'/>
+    <controller type='scsi' index='0'>
+      <address type='spapr-vio' reg='0x2000'/>
+    </controller>
     <controller type='pci' index='0' model='pci-root'/>
     <input type='keyboard' bus='usb'/>
     <input type='mouse' bus='usb'/>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 63480ce..fb9630d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -304,14 +304,6 @@  static int testCompareXMLToArgvFiles(const char *xml,
 
     virQEMUCapsFilterByMachineType(extraFlags, vmdef->os.machine);
 
-    if (virQEMUCapsGet(extraFlags, QEMU_CAPS_DEVICE)) {
-        if (qemuDomainAssignAddresses(vmdef, extraFlags, NULL)) {
-            if (flags & FLAG_EXPECT_ERROR)
-                goto ok;
-            goto out;
-        }
-    }
-
     log = virtTestLogContentAndReset();
     VIR_FREE(log);
     virResetLastError();
@@ -1373,7 +1365,7 @@  mymain(void)
             QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION);
     DO_TEST("pseries-vio-user-assigned",
             QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
-    DO_TEST_ERROR("pseries-vio-address-clash",
+    DO_TEST_PARSE_ERROR("pseries-vio-address-clash",
             QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
     DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM);
     DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI,
@@ -1499,7 +1491,7 @@  mymain(void)
             QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
             QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
 
-    DO_TEST_ERROR("pcie-root-port-too-many",
+    DO_TEST_PARSE_ERROR("pcie-root-port-too-many",
             QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
             QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
             QEMU_CAPS_DEVICE_IOH3420,
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
index 7a608a8..b300324 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml
@@ -29,10 +29,11 @@ 
     <controller type='virtio-serial' index='2'/>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='1'/>
     </channel>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.foo'/>
-      <address type='virtio-serial' controller='1' bus='0' port='0'/>
+      <address type='virtio-serial' controller='1' bus='0' port='1'/>
     </channel>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.bar'/>
@@ -40,15 +41,15 @@ 
     </channel>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.wizz'/>
-      <address type='virtio-serial' controller='0' bus='0' port='0'/>
+      <address type='virtio-serial' controller='0' bus='0' port='2'/>
     </channel>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.ooh'/>
-      <address type='virtio-serial' controller='1' bus='0' port='0'/>
+      <address type='virtio-serial' controller='1' bus='0' port='2'/>
     </channel>
     <channel type='pty'>
       <target type='virtio' name='org.linux-kvm.port.lla'/>
-      <address type='virtio-serial' controller='2' bus='0' port='0'/>
+      <address type='virtio-serial' controller='2' bus='0' port='1'/>
     </channel>
     <memballoon model='virtio'/>
   </devices>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml
new file mode 100644
index 0000000..6bf4ca5
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml
@@ -0,0 +1,35 @@ 
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='i686' 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</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <source file='/tmp/scsidisk.img'/>
+      <target dev='sda' bus='scsi'/>
+      <address type='drive' controller='0' bus='0' target='3' unit='0'/>
+    </disk>
+    <controller type='ide' index='0'/>
+    <controller type='scsi' index='0' model='ibmvscsi'>
+      <address type='spapr-vio' reg='0x2000'/>
+    </controller>
+    <controller type='usb' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml
new file mode 100644
index 0000000..81d81dd
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml
@@ -0,0 +1,30 @@ 
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <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'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <serial type='pty'>
+      <target port='0'/>
+      <address type='spapr-vio' reg='0x30000000'/>
+    </serial>
+    <console type='pty'>
+      <target type='serial' port='0'/>
+      <address type='spapr-vio' reg='0x30000000'/>
+    </console>
+    <memballoon model='none'/>
+    <panic model='pseries'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
index 8fcd644..81d81dd 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml
@@ -18,11 +18,11 @@ 
     <controller type='pci' index='0' model='pci-root'/>
     <serial type='pty'>
       <target port='0'/>
-      <address type='spapr-vio'/>
+      <address type='spapr-vio' reg='0x30000000'/>
     </serial>
     <console type='pty'>
       <target type='serial' port='0'/>
-      <address type='spapr-vio'/>
+      <address type='spapr-vio' reg='0x30000000'/>
     </console>
     <memballoon model='none'/>
     <panic model='pseries'/>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml
index 8fcd644..81d81dd 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml
@@ -18,11 +18,11 @@ 
     <controller type='pci' index='0' model='pci-root'/>
     <serial type='pty'>
       <target port='0'/>
-      <address type='spapr-vio'/>
+      <address type='spapr-vio' reg='0x30000000'/>
     </serial>
     <console type='pty'>
       <target type='serial' port='0'/>
-      <address type='spapr-vio'/>
+      <address type='spapr-vio' reg='0x30000000'/>
     </console>
     <memballoon model='none'/>
     <panic model='pseries'/>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 32c9fed..393acb7 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -232,6 +232,12 @@  testInfoSet(struct testInfo *info,
     if (!(info->qemuCaps = virQEMUCapsNew()))
         goto error;
 
+    virQEMUCapsSetList(info->qemuCaps,
+                       QEMU_CAPS_SCSI_LSI,
+                       QEMU_CAPS_VIRTIO_SCSI,
+                       QEMU_CAPS_SCSI_MEGASAS,
+                       QEMU_CAPS_LAST);
+
     if (qemuTestCapsCacheInsert(driver.qemuCapsCache, name,
                                 info->qemuCaps) < 0)
         goto error;
@@ -417,7 +423,7 @@  mymain(void)
     DO_TEST("disk-drive-network-iscsi");
     DO_TEST("disk-drive-network-iscsi-auth");
     DO_TEST("disk-scsi-device");
-    DO_TEST("disk-scsi-vscsi");
+    DO_TEST_DIFFERENT("disk-scsi-vscsi");
     DO_TEST("disk-scsi-virtio-scsi");
     DO_TEST("disk-virtio-scsi-num_queues");
     DO_TEST("disk-virtio-scsi-cmd_per_lun");
@@ -602,7 +608,7 @@  mymain(void)
 
     DO_TEST_DIFFERENT("panic");
     DO_TEST("panic-isa");
-    DO_TEST("panic-pseries");
+    DO_TEST_DIFFERENT("panic-pseries");
     DO_TEST("panic-double");
     DO_TEST("panic-no-address");