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

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




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

Would this be useful if "multifunction='on'" without specifying an
address? e.g.:

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


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.

Rather than focus on anything that could change if patch3 is adjusted...
I'll point only a couple of things here. I do think the less times to
iterate through devices the better - the whole address assignment
processing and post part device iteration is complex enough already!

John

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

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

>      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);
> 


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