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