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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list