[6/8] domain: Make <address type='pci'/> request address allocation

Message ID 0ca25abc9ce0847397068860c18d57f624380e4d.1457454944.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson March 8, 2016, 4:36 p.m.
If a bare device <address type='pci'/> is specified, set an internal
flag address->auto_allocate. Individual hv drivers can then check for
this and act on it if they want, nothing is allocated in generic code.

If drivers allocate an address, they are expected to unset auto_allocate.
Generic domain conf code then checks at XML format time to ensure no
device addresses still have auto_allocate set; this ensures we aren't
formatting any bogus address XML, and informing the user if their
request didn't work. Add a genericxml2xml test case for this.

The auto_allocate property is a part of the generic address structure
and not the PCI specific bits, this will make it easier to reuse with
other address types too.

One note: we detect <address type='pci'/> by counting it's XML properties,
rather than comparing specifically against parsed values, which seems
easier to maintain.
---
 docs/schemas/domaincommon.rng                      |  5 +++-
 src/conf/domain_conf.c                             | 29 +++++++++++++++++++++-
 src/conf/domain_conf.h                             |  1 +
 .../generic-pci-autofill-addr.xml                  | 27 ++++++++++++++++++++
 tests/genericxml2xmltest.c                         | 17 +++++++++----
 5 files changed, 72 insertions(+), 7 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/generic-pci-autofill-addr.xml

-- 
2.5.0

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

Comments

Cole Robinson March 21, 2016, 6:59 p.m. | #1
On 03/21/2016 02:44 PM, John Ferlan wrote:
> 

> 

> On 03/08/2016 11:36 AM, Cole Robinson wrote:

>> If a bare device <address type='pci'/> is specified, set an internal

>> flag address->auto_allocate. Individual hv drivers can then check for

>> this and act on it if they want, nothing is allocated in generic code.

>>

>> If drivers allocate an address, they are expected to unset auto_allocate.

>> Generic domain conf code then checks at XML format time to ensure no

>> device addresses still have auto_allocate set; this ensures we aren't

>> formatting any bogus address XML, and informing the user if their

>> request didn't work. Add a genericxml2xml test case for this.

>>

>> The auto_allocate property is a part of the generic address structure

>> and not the PCI specific bits, this will make it easier to reuse with

>> other address types too.

>>

>> One note: we detect <address type='pci'/> by counting it's XML properties,

>> rather than comparing specifically against parsed values, which seems

>> easier to maintain.

>> ---

>>  docs/schemas/domaincommon.rng                      |  5 +++-

>>  src/conf/domain_conf.c                             | 29 +++++++++++++++++++++-

>>  src/conf/domain_conf.h                             |  1 +

>>  .../generic-pci-autofill-addr.xml                  | 27 ++++++++++++++++++++

>>  tests/genericxml2xmltest.c                         | 17 +++++++++----

>>  5 files changed, 72 insertions(+), 7 deletions(-)

>>  create mode 100644 tests/genericxml2xmlindata/generic-pci-autofill-addr.xml

>>

> 

> Will also need to update 'formatdomain.html.in' to describe the new

> allowance of just "<address type='pci'>...

> 


Darn I must have remembered and then forgot to update the docs like 10 times :/

> Would this be useful if "multifunction='on'" without specifying an

> address? e.g.:

> 

>     <address type='pci' multifunction='on'>

> 


Maybe? Honestly I don't know much about when multifunction should be used.

> 

> Could other address types find the functionality useful? That is, I want

> address type 'drive' or 'usb', but I have no idea how to fill in the

> rest, I want the hv to do it for me.

> 


My intent with how this was implemented is that it shouldn't take much change
in generic domain_conf.c code to handle this for other address types: position
of auto_allocate in the address structure, and the generic device iterator
validating that all addresses were allocated.
> 

>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng

>> index 6ca937c..d083250 100644

>> --- a/docs/schemas/domaincommon.rng

>> +++ b/docs/schemas/domaincommon.rng

>> @@ -4471,7 +4471,10 @@

>>            <attribute name="type">

>>              <value>pci</value>

>>            </attribute>

>> -          <ref name="pciaddress"/>

>> +          <choice>

>> +            <ref name="pciaddress"/>

>> +            <empty/>

>> +          </choice>

>>          </group>

>>          <group>

>>            <attribute name="type">

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c

>> index bc4e369..bbc42a4 100644

>> --- a/src/conf/domain_conf.c

>> +++ b/src/conf/domain_conf.c

>> @@ -3827,6 +3827,23 @@ virDomainDefPostParseTimer(virDomainDefPtr def)

>>  }

>>  

>>  

>> + static int

>    ^

> Extraneous space

> 

>> +virDomainCheckUnallocatedDeviceAddrs(virDomainDefPtr def ATTRIBUTE_UNUSED,

>> +                                     virDomainDeviceDefPtr dev,

>> +                                     virDomainDeviceInfoPtr info,

>> +                                     void *data ATTRIBUTE_UNUSED)

>> +{

>> +    if (!info->auto_allocate)

>> +        return 0;

>> +

>> +    virReportError(VIR_ERR_INTERNAL_ERROR,

>> +        _("driver didn't allocate requested address type '%s' for device '%s'"),

>> +        virDomainDeviceAddressTypeToString(info->type),

>> +        virDomainDeviceTypeToString(dev->type));

>> +    return -1;

>> +}

>> +

>> +

>>  static int

>>  virDomainDefPostParseInternal(virDomainDefPtr def,

>>                                virCapsPtr caps ATTRIBUTE_UNUSED,

>> @@ -3851,6 +3868,11 @@ virDomainDefPostParseInternal(virDomainDefPtr def,

>>      if (virDomainDefPostParseTimer(def) < 0)

>>          return -1;

>>  

>> +    /* ensure the driver filled in any auto_allocate addrs */

>> +    if (virDomainDeviceInfoIterate(def, virDomainCheckUnallocatedDeviceAddrs,

>> +                                   NULL) < 0)

>> +        return -1;

>> +

> 

> Why couldn't this go in virDomainDefPostParseDeviceIterator?

> 

> That is, rather than have yet another iterator through the devices, call

> virDomainCheckUnallocatedDeviceAddrs right after or at the end of

> virDomainDeviceDefPostParse...

> 


As the patches stand, it won't work with due to the ordering: this validation
check needs to come after qemuDomainAssignAddresses. When the patches are
reworked it may be able to be combined with DeviceDefPostParse

>>      if (virDomainDefAddImplicitDevices(def) < 0)

>>          return -1;

>>  

>> @@ -4872,8 +4894,13 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,

>>  

>>      switch ((virDomainDeviceAddressType) info->type) {

>>      case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:

>> -        if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0)

>> +        if (virXMLPropertyCount(address) == 1) {

> 

> Wish there was some other way than counting attributes, but I guess

> since "type" is an attribute we can hopefully guarantee that only

> "type=<something>" has been supplied.

> 

>> +            /* Bare <address type='pci'/> is a request to allocate

>> +               the address. */

>> +            info->auto_allocate = true;

>> +        } else if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) {

>>              goto cleanup;

>> +        }

>>          break;

>>  

>>      case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:

>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h

>> index aba53a2..dd9d0b1 100644

>> --- a/src/conf/domain_conf.h

>> +++ b/src/conf/domain_conf.h

>> @@ -346,6 +346,7 @@ struct _virDomainDeviceInfo {

>>       */

>>      char *alias;

>>      int type; /* virDomainDeviceAddressType */

>> +    bool auto_allocate;

> 

> The name is generic enough to make me think it could work for other

> address types, but only PCI is supported.


Yup, that's what I was going for.

- Cole

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

Patch hide | download patch | download mbox

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6ca937c..d083250 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4471,7 +4471,10 @@ 
           <attribute name="type">
             <value>pci</value>
           </attribute>
-          <ref name="pciaddress"/>
+          <choice>
+            <ref name="pciaddress"/>
+            <empty/>
+          </choice>
         </group>
         <group>
           <attribute name="type">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bc4e369..bbc42a4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3827,6 +3827,23 @@  virDomainDefPostParseTimer(virDomainDefPtr def)
 }
 
 
+ static int
+virDomainCheckUnallocatedDeviceAddrs(virDomainDefPtr def ATTRIBUTE_UNUSED,
+                                     virDomainDeviceDefPtr dev,
+                                     virDomainDeviceInfoPtr info,
+                                     void *data ATTRIBUTE_UNUSED)
+{
+    if (!info->auto_allocate)
+        return 0;
+
+    virReportError(VIR_ERR_INTERNAL_ERROR,
+        _("driver didn't allocate requested address type '%s' for device '%s'"),
+        virDomainDeviceAddressTypeToString(info->type),
+        virDomainDeviceTypeToString(dev->type));
+    return -1;
+}
+
+
 static int
 virDomainDefPostParseInternal(virDomainDefPtr def,
                               virCapsPtr caps ATTRIBUTE_UNUSED,
@@ -3851,6 +3868,11 @@  virDomainDefPostParseInternal(virDomainDefPtr def,
     if (virDomainDefPostParseTimer(def) < 0)
         return -1;
 
+    /* ensure the driver filled in any auto_allocate addrs */
+    if (virDomainDeviceInfoIterate(def, virDomainCheckUnallocatedDeviceAddrs,
+                                   NULL) < 0)
+        return -1;
+
     if (virDomainDefAddImplicitDevices(def) < 0)
         return -1;
 
@@ -4872,8 +4894,13 @@  virDomainDeviceInfoParseXML(xmlNodePtr node,
 
     switch ((virDomainDeviceAddressType) info->type) {
     case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
-        if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0)
+        if (virXMLPropertyCount(address) == 1) {
+            /* Bare <address type='pci'/> is a request to allocate
+               the address. */
+            info->auto_allocate = true;
+        } else if (virDevicePCIAddressParseXML(address, &info->addr.pci) < 0) {
             goto cleanup;
+        }
         break;
 
     case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aba53a2..dd9d0b1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -346,6 +346,7 @@  struct _virDomainDeviceInfo {
      */
     char *alias;
     int type; /* virDomainDeviceAddressType */
+    bool auto_allocate;
     union {
         virDevicePCIAddress pci;
         virDomainDeviceDriveAddress drive;
diff --git a/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml b/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml
new file mode 100644
index 0000000..06eadb6
--- /dev/null
+++ b/tests/genericxml2xmlindata/generic-pci-autofill-addr.xml
@@ -0,0 +1,27 @@ 
+<domain type='test'>
+  <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'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='vda' bus='virtio'/>
+      <address type='pci'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci'/>
+    </controller>
+  </devices>
+</domain>
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 666fc86..b329d10 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -21,6 +21,7 @@  struct testInfo {
     const char *name;
     int different;
     bool inactive_only;
+    testCompareDomXML2XMLFlags compare_flags;
 };
 
 static int
@@ -39,7 +40,7 @@  testCompareXMLToXMLHelper(const void *data)
 
     ret = testCompareDomXML2XMLFiles(caps, xmlopt, xml_in,
                                      info->different ? xml_out : xml_in,
-                                     !info->inactive_only, 0,
+                                     !info->inactive_only, info->compare_flags,
                                      NULL, NULL, 0);
  cleanup:
     VIR_FREE(xml_in);
@@ -59,21 +60,27 @@  mymain(void)
     if (!(xmlopt = virTestGenericDomainXMLConfInit()))
         return EXIT_FAILURE;
 
-#define DO_TEST_FULL(name, is_different, inactive)                      \
+#define DO_TEST_FULL(name, is_different, inactive, compare_flags)       \
     do {                                                                \
-        const struct testInfo info = {name, is_different, inactive};    \
+        const struct testInfo info = {name, is_different,               \
+                                      inactive, compare_flags};         \
         if (virtTestRun("GENERIC XML-2-XML " name,                      \
                         testCompareXMLToXMLHelper, &info) < 0)          \
             ret = -1;                                                   \
     } while (0)
 
 #define DO_TEST(name) \
-    DO_TEST_FULL(name, 0, false)
+    DO_TEST_FULL(name, 0, false, 0)
+
+#define DO_TEST_PARSE_ERROR(name) \
+    DO_TEST_FULL(name, 0, false,  \
+                 TEST_COMPARE_DOM_XML2XML_FLAG_EXPECT_PARSE_ERROR)
 
 #define DO_TEST_DIFFERENT(name) \
-    DO_TEST_FULL(name, 1, false)
+    DO_TEST_FULL(name, 1, false, 0)
 
     DO_TEST_DIFFERENT("disk-virtio");
+    DO_TEST_PARSE_ERROR("pci-autofill-addr");
 
     virObjectUnref(caps);
     virObjectUnref(xmlopt);