| Message ID | 0ca25abc9ce0847397068860c18d57f624380e4d.1457454944.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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);