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

[Cole Robinson]

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