[libvirt] [PATCH v2 0/4] Fix parsing our own XMLs
v2: - Just a rebase - I did *not* use virPCIDeviceAddress wording instead as discussed in the v1 thread. That's because we have lot of functions working with virDevicePCIAddress named exactly after that and renaming those would be ugly IMHO. v1: - https://www.redhat.com/archives/libvir-list/2016-April/msg00081.html Martin Kletzander (4): Change virPCIDeviceAddress to virDevicePCIAddress Move capability formatting together schemas: Update nodedev schema to match reality conf: Parse more of our nodedev XML docs/schemas/nodedev.rng | 29 +++--- src/conf/device_conf.h | 11 +-- src/conf/node_device_conf.c | 109 +++++++++++++++++++-- src/conf/node_device_conf.h | 6 +- src/libvirt_private.syms | 10 +- src/network/bridge_driver.c | 4 +- src/node_device/node_device_linux_sysfs.c | 6 +- src/util/virhostdev.c | 12 +-- src/util/virnetdev.c | 4 +- src/util/virnetdev.h | 2 +- src/util/virpci.c | 80 +++++++-------- src/util/virpci.h | 29 +++--- .../pci_0000_00_1c_0_header_type.xml | 2 +- tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml | 23 +++++ .../pci_0000_02_10_7_sriov_pf_vfs_all.xml | 29 ++++++ ...i_0000_02_10_7_sriov_pf_vfs_all_header_type.xml | 30 ++++++ .../pci_0000_02_10_7_sriov_vfs.xml | 26 +++++ .../pci_0000_02_10_7_sriov_zero_vfs_max_count.xml | 21 ++++ tests/nodedevxml2xmltest.c | 7 ++ 19 files changed, 335 insertions(+), 105 deletions(-) create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_vfs.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_zero_vfs_max_count.xml -- 2.8.1
We had both and the only difference was that the latter also included information about multifunction setting. The problem with that was that we couldn't use functions made for only one of the structs (e.g. parsing). To consolidate those two structs, move it to virpci.h and include that in domain_conf.h. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/device_conf.h | 11 +---- src/conf/node_device_conf.c | 2 +- src/conf/node_device_conf.h | 6 +-- src/libvirt_private.syms | 10 ++-- src/network/bridge_driver.c | 4 +- src/node_device/node_device_linux_sysfs.c | 6 +-- src/util/virhostdev.c | 12 ++--- src/util/virnetdev.c | 4 +- src/util/virnetdev.h | 2 +- src/util/virpci.c | 80 +++++++++++++++---------------- src/util/virpci.h | 29 +++++------ 11 files changed, 79 insertions(+), 87 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 85ce40f6831e..3fe259cb8916 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -31,6 +31,7 @@ # include "virutil.h" # include "virthread.h" # include "virbuffer.h" +# include "virpci.h" typedef enum { VIR_INTERFACE_STATE_UNKNOWN = 1, @@ -45,16 +46,6 @@ typedef enum { VIR_ENUM_DECL(virInterfaceState) -typedef struct _virDevicePCIAddress virDevicePCIAddress; -typedef virDevicePCIAddress *virDevicePCIAddressPtr; -struct _virDevicePCIAddress { - unsigned int domain; - unsigned int bus; - unsigned int slot; - unsigned int function; - int multi; /* virTristateSwitch */ -}; - typedef struct _virInterfaceLink virInterfaceLink; typedef virInterfaceLink *virInterfaceLinkPtr; struct _virInterfaceLink { diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a76f785eddc0..3e9c821762eb 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1142,7 +1142,7 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr ctxt, char *numberStr = NULL; int nAddrNodes, ret = -1; size_t i; - virPCIDeviceAddressPtr pciAddr = NULL; + virDevicePCIAddressPtr pciAddr = NULL; ctxt->node = iommuGroupNode; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index be6dd5eb4ec1..9c9d942dd506 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -109,12 +109,12 @@ typedef struct _virNodeDevCapData { unsigned int class; char *product_name; char *vendor_name; - virPCIDeviceAddressPtr physical_function; - virPCIDeviceAddressPtr *virtual_functions; + virDevicePCIAddressPtr physical_function; + virDevicePCIAddressPtr *virtual_functions; size_t num_virtual_functions; unsigned int max_virtual_functions; unsigned int flags; - virPCIDeviceAddressPtr *iommuGroupDevices; + virDevicePCIAddressPtr *iommuGroupDevices; size_t nIommuGroupDevices; unsigned int iommuGroupNumber; int numa_node; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a9719ea169ca..e07b2cb02647 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1966,11 +1966,11 @@ virObjectUnref; # util/virpci.h -virPCIDeviceAddressGetIOMMUGroupAddresses; -virPCIDeviceAddressGetIOMMUGroupNum; -virPCIDeviceAddressGetSysfsFile; -virPCIDeviceAddressIOMMUGroupIterate; -virPCIDeviceAddressParse; +virDevicePCIAddressGetIOMMUGroupAddresses; +virDevicePCIAddressGetIOMMUGroupNum; +virDevicePCIAddressGetSysfsFile; +virDevicePCIAddressIOMMUGroupIterate; +virDevicePCIAddressParse; virPCIDeviceCopy; virPCIDeviceDetach; virPCIDeviceFileIterate; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 73236ffe1cd9..751da77cf5b2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2336,7 +2336,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) size_t numVirtFns = 0; unsigned int maxVirtFns = 0; char **vfNames = NULL; - virPCIDeviceAddressPtr *virtFns; + virDevicePCIAddressPtr *virtFns; int ret = -1; size_t i; @@ -2356,7 +2356,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) goto cleanup; for (i = 0; i < numVirtFns; i++) { - virPCIDeviceAddressPtr thisVirtFn = virtFns[i]; + virDevicePCIAddressPtr thisVirtFn = virtFns[i]; const char *thisName = vfNames[i]; virNetworkForwardIfDefPtr thisIf = &netdef->forward.ifs[netdef->forward.nifs]; diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 24a6a2eaa51f..0a858d31f387 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -176,7 +176,7 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapDataPtr data) { size_t i; int tmpGroup, ret = -1; - virPCIDeviceAddress addr; + virDevicePCIAddress addr; /* this could be a refresh, so clear out the old data */ for (i = 0; i < data->pci_dev.nIommuGroupDevices; i++) @@ -189,7 +189,7 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapDataPtr data) addr.bus = data->pci_dev.bus; addr.slot = data->pci_dev.slot; addr.function = data->pci_dev.function; - tmpGroup = virPCIDeviceAddressGetIOMMUGroupNum(&addr); + tmpGroup = virDevicePCIAddressGetIOMMUGroupNum(&addr); if (tmpGroup == -1) { /* error was already reported */ goto cleanup; @@ -200,7 +200,7 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapDataPtr data) goto cleanup; } if (tmpGroup >= 0) { - if (virPCIDeviceAddressGetIOMMUGroupAddresses(&addr, &data->pci_dev.iommuGroupDevices, + if (virDevicePCIAddressGetIOMMUGroupAddresses(&addr, &data->pci_dev.iommuGroupDevices, &data->pci_dev.nIommuGroupDevices) < 0) goto cleanup; data->pci_dev.iommuGroupNumber = tmpGroup; diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 933c94263c58..1c2475356c4a 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -68,7 +68,7 @@ struct virHostdevIsPCINodeDeviceUsedData { * * pci - a short-lived virPCIDevice whose purpose is usually just to look * up the actual PCI device in one of the bookkeeping lists; basically - * little more than a fancy virPCIDeviceAddress + * little more than a fancy virDevicePCIAddress * * pcidevs - a list containing a bunch of the above * @@ -81,7 +81,7 @@ struct virHostdevIsPCINodeDeviceUsedData { * module usually expect an 'actual'. Even with these conventions in place, * adding comments to highlight ownership-related issues is recommended */ -static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) +static int virHostdevIsPCINodeDeviceUsed(virDevicePCIAddressPtr devAddr, void *opaque) { virPCIDevicePtr actual; int ret = -1; @@ -270,14 +270,14 @@ static int virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) { - virPCIDeviceAddress config_address; + virDevicePCIAddress config_address; config_address.domain = hostdev->source.subsys.u.pci.addr.domain; config_address.bus = hostdev->source.subsys.u.pci.addr.bus; config_address.slot = hostdev->source.subsys.u.pci.addr.slot; config_address.function = hostdev->source.subsys.u.pci.addr.function; - return virPCIDeviceAddressGetSysfsFile(&config_address, sysfs_path); + return virDevicePCIAddressGetSysfsFile(&config_address, sysfs_path); } @@ -508,7 +508,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, int last_processed_hostdev_vf = -1; size_t i; int ret = -1; - virPCIDeviceAddressPtr devAddr = NULL; + virDevicePCIAddressPtr devAddr = NULL; if (!nhostdevs) return 0; @@ -548,7 +548,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, */ devAddr = virPCIDeviceGetAddress(pci); if (usesVFIO) { - if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, + if (virDevicePCIAddressIOMMUGroupIterate(devAddr, virHostdevIsPCINodeDeviceUsed, &data) < 0) goto cleanup; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a505b6c0b627..f83a7af730f2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1830,7 +1830,7 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname, int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, - virPCIDeviceAddressPtr **virt_fns, + virDevicePCIAddressPtr **virt_fns, size_t *n_vfname, unsigned int *max_vfs) { @@ -2017,7 +2017,7 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, int virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED, char ***vfname ATTRIBUTE_UNUSED, - virPCIDeviceAddressPtr **virt_fns ATTRIBUTE_UNUSED, + virDevicePCIAddressPtr **virt_fns ATTRIBUTE_UNUSED, size_t *n_vfname ATTRIBUTE_UNUSED, unsigned int *max_vfs ATTRIBUTE_UNUSED) { diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 240fff774d30..948b4cf0edb9 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -163,7 +163,7 @@ int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) int virNetDevGetVirtualFunctions(const char *pfname, char ***vfname, - virPCIDeviceAddressPtr **virt_fns, + virDevicePCIAddressPtr **virt_fns, size_t *n_vfname, unsigned int *max_vfs) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) diff --git a/src/util/virpci.c b/src/util/virpci.c index f7921f86d6de..215a03efa670 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -69,7 +69,7 @@ VIR_ENUM_IMPL(virPCIHeader, VIR_PCI_HEADER_LAST, ); struct _virPCIDevice { - virPCIDeviceAddress address; + virDevicePCIAddress address; char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ @@ -1696,7 +1696,7 @@ virPCIDeviceFree(virPCIDevicePtr dev) * * Returns: a pointer to the address, which can never be NULL. */ -virPCIDeviceAddressPtr +virDevicePCIAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev) { return &(dev->address); @@ -2001,14 +2001,14 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, } -/* virPCIDeviceAddressIOMMUGroupIterate: +/* virDevicePCIAddressIOMMUGroupIterate: * Call @actor for all devices in the same iommu_group as orig * (including orig itself) Even if there is no iommu_group for the * device, call @actor once for orig. */ int -virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, - virPCIDeviceAddressActor actor, +virDevicePCIAddressIOMMUGroupIterate(virDevicePCIAddressPtr orig, + virDevicePCIAddressActor actor, void *opaque) { char *groupPath = NULL; @@ -2029,12 +2029,12 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, } while ((direrr = virDirRead(groupDir, &ent, groupPath)) > 0) { - virPCIDeviceAddress newDev; + virDevicePCIAddress newDev; if (ent->d_name[0] == '.') continue; - if (virPCIDeviceAddressParse(ent->d_name, &newDev) < 0) { + if (virDevicePCIAddressParse(ent->d_name, &newDev) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Found invalid device link '%s' in '%s'"), ent->d_name, groupPath); @@ -2058,7 +2058,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, static int -virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) +virPCIDeviceGetIOMMUGroupAddOne(virDevicePCIAddressPtr newDevAddr, void *opaque) { int ret = -1; virPCIDeviceListPtr groupList = opaque; @@ -2093,7 +2093,7 @@ virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev) if (!groupList) goto error; - if (virPCIDeviceAddressIOMMUGroupIterate(&(dev->address), + if (virDevicePCIAddressIOMMUGroupIterate(&(dev->address), virPCIDeviceGetIOMMUGroupAddOne, groupList) < 0) goto error; @@ -2107,17 +2107,17 @@ virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev) typedef struct { - virPCIDeviceAddressPtr **iommuGroupDevices; + virDevicePCIAddressPtr **iommuGroupDevices; size_t *nIommuGroupDevices; -} virPCIDeviceAddressList; -typedef virPCIDeviceAddressList *virPCIDeviceAddressListPtr; +} virDevicePCIAddressList; +typedef virDevicePCIAddressList *virDevicePCIAddressListPtr; static int -virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) +virPCIGetIOMMUGroupAddressesAddOne(virDevicePCIAddressPtr newDevAddr, void *opaque) { int ret = -1; - virPCIDeviceAddressListPtr addrList = opaque; - virPCIDeviceAddressPtr copyAddr; + virDevicePCIAddressListPtr addrList = opaque; + virDevicePCIAddressPtr copyAddr; /* make a copy to insert onto the list */ if (VIR_ALLOC(copyAddr) < 0) @@ -2137,22 +2137,22 @@ virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaq /* - * virPCIDeviceAddressGetIOMMUGroupAddresses - return a + * virDevicePCIAddressGetIOMMUGroupAddresses - return a * virPCIDeviceList containing all of the devices in the same * iommu_group as @dev. * * Return the new list, or NULL on failure */ int -virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr, - virPCIDeviceAddressPtr **iommuGroupDevices, +virDevicePCIAddressGetIOMMUGroupAddresses(virDevicePCIAddressPtr devAddr, + virDevicePCIAddressPtr **iommuGroupDevices, size_t *nIommuGroupDevices) { int ret = -1; - virPCIDeviceAddressList addrList = { iommuGroupDevices, + virDevicePCIAddressList addrList = { iommuGroupDevices, nIommuGroupDevices }; - if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, + if (virDevicePCIAddressIOMMUGroupIterate(devAddr, virPCIGetIOMMUGroupAddressesAddOne, &addrList) < 0) goto cleanup; @@ -2163,12 +2163,12 @@ virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr, } -/* virPCIDeviceAddressGetIOMMUGroupNum - return the group number of +/* virDevicePCIAddressGetIOMMUGroupNum - return the group number of * this PCI device's iommu_group, or -2 if there is no iommu_group for * the device (or -1 if there was any other error) */ int -virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) +virDevicePCIAddressGetIOMMUGroupNum(virDevicePCIAddressPtr addr) { char *devName = NULL; char *devPath = NULL; @@ -2391,8 +2391,8 @@ logStrToLong_ui(char const *s, } int -virPCIDeviceAddressParse(char *address, - virPCIDeviceAddressPtr bdf) +virDevicePCIAddressParse(char *address, + virDevicePCIAddressPtr bdf) { char *p = NULL; int ret = -1; @@ -2429,8 +2429,8 @@ virPCIDeviceAddressParse(char *address, * returns true if equal */ static bool -virPCIDeviceAddressIsEqual(virPCIDeviceAddressPtr bdf1, - virPCIDeviceAddressPtr bdf2) +virDevicePCIAddressIsEqual(virDevicePCIAddressPtr bdf1, + virDevicePCIAddressPtr bdf2) { return ((bdf1->domain == bdf2->domain) && (bdf1->bus == bdf2->bus) && @@ -2440,7 +2440,7 @@ virPCIDeviceAddressIsEqual(virPCIDeviceAddressPtr bdf1, static int virPCIGetDeviceAddressFromSysfsLink(const char *device_link, - virPCIDeviceAddressPtr *bdf) + virDevicePCIAddressPtr *bdf) { char *config_address = NULL; char *device_path = NULL; @@ -2465,7 +2465,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link, if (VIR_ALLOC(*bdf) != 0) goto out; - if (virPCIDeviceAddressParse(config_address, *bdf) != 0) { + if (virDevicePCIAddressParse(config_address, *bdf) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse PCI config address '%s'"), config_address); @@ -2485,7 +2485,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link, */ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, - virPCIDeviceAddressPtr *pf) + virDevicePCIAddressPtr *pf) { int ret = -1; char *device_link = NULL; @@ -2510,14 +2510,14 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, */ int virPCIGetVirtualFunctions(const char *sysfs_path, - virPCIDeviceAddressPtr **virtual_functions, + virDevicePCIAddressPtr **virtual_functions, size_t *num_virtual_functions, unsigned int *max_virtual_functions) { int ret = -1; size_t i; char *device_link = NULL; - virPCIDeviceAddress *config_addr = NULL; + virDevicePCIAddressPtr config_addr = NULL; char *totalvfs_file = NULL, *totalvfs_str = NULL; *virtual_functions = NULL; @@ -2609,8 +2609,8 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, size_t i; size_t num_virt_fns = 0; unsigned int max_virt_fns = 0; - virPCIDeviceAddressPtr vf_bdf = NULL; - virPCIDeviceAddressPtr *virt_fns = NULL; + virDevicePCIAddressPtr vf_bdf = NULL; + virDevicePCIAddressPtr *virt_fns = NULL; if (virPCIGetDeviceAddressFromSysfsLink(vf_sysfs_device_link, &vf_bdf) < 0) @@ -2625,7 +2625,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, } for (i = 0; i < num_virt_fns; i++) { - if (virPCIDeviceAddressIsEqual(vf_bdf, virt_fns[i])) { + if (virDevicePCIAddressIsEqual(vf_bdf, virt_fns[i])) { *vf_index = i; ret = 0; break; @@ -2658,7 +2658,7 @@ virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link) } int -virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, +virDevicePCIAddressGetSysfsFile(virDevicePCIAddressPtr addr, char **pci_sysfs_device_link) { if (virAsprintf(pci_sysfs_device_link, @@ -2713,14 +2713,14 @@ int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, char **pfname, int *vf_index) { - virPCIDeviceAddressPtr pf_config_address = NULL; + virDevicePCIAddressPtr pf_config_address = NULL; char *pf_sysfs_device_path = NULL; int ret = -1; if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) return ret; - if (virPCIDeviceAddressGetSysfsFile(pf_config_address, + if (virDevicePCIAddressGetSysfsFile(pf_config_address, &pf_sysfs_device_path) < 0) { VIR_FREE(pf_config_address); @@ -2745,7 +2745,7 @@ static const char *unsupported = N_("not supported on non-linux platforms"); int virPCIGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, - virPCIDeviceAddressPtr *pf ATTRIBUTE_UNUSED) + virDevicePCIAddressPtr *pf ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; @@ -2753,7 +2753,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path ATTRIBUTE_UNUSED, int virPCIGetVirtualFunctions(const char *sysfs_path ATTRIBUTE_UNUSED, - virPCIDeviceAddressPtr **virtual_functions ATTRIBUTE_UNUSED, + virDevicePCIAddressPtr **virtual_functions ATTRIBUTE_UNUSED, size_t *num_virtual_functions ATTRIBUTE_UNUSED, unsigned int *max_virtual_functions ATTRIBUTE_UNUSED) { @@ -2779,7 +2779,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link ATTRIBUTE_UNUSED, } int -virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr dev ATTRIBUTE_UNUSED, +virDevicePCIAddressGetSysfsFile(virDevicePCIAddressPtr dev ATTRIBUTE_UNUSED, char **pci_sysfs_device_link ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); diff --git a/src/util/virpci.h b/src/util/virpci.h index 82f45ec4175f..8ae916c562d7 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -30,16 +30,17 @@ typedef struct _virPCIDevice virPCIDevice; typedef virPCIDevice *virPCIDevicePtr; -typedef struct _virPCIDeviceAddress virPCIDeviceAddress; -typedef virPCIDeviceAddress *virPCIDeviceAddressPtr; typedef struct _virPCIDeviceList virPCIDeviceList; typedef virPCIDeviceList *virPCIDeviceListPtr; -struct _virPCIDeviceAddress { +typedef struct _virDevicePCIAddress virDevicePCIAddress; +typedef virDevicePCIAddress *virDevicePCIAddressPtr ; +struct _virDevicePCIAddress { unsigned int domain; unsigned int bus; unsigned int slot; unsigned int function; + int multi; /* virTristateSwitch */ }; typedef enum { @@ -113,7 +114,7 @@ bool virPCIDeviceGetManaged(virPCIDevice *dev); void virPCIDeviceSetStubDriver(virPCIDevicePtr dev, virPCIStubDriver driver); virPCIStubDriver virPCIDeviceGetStubDriver(virPCIDevicePtr dev); -virPCIDeviceAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev); +virDevicePCIAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev); int virPCIDeviceSetUsedBy(virPCIDevice *dev, const char *drv_name, const char *dom_name); @@ -168,16 +169,16 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, virPCIDeviceFileActor actor, void *opaque); -typedef int (*virPCIDeviceAddressActor)(virPCIDeviceAddress *addr, +typedef int (*virDevicePCIAddressActor)(virDevicePCIAddressPtr addr, void *opaque); -int virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, - virPCIDeviceAddressActor actor, +int virDevicePCIAddressIOMMUGroupIterate(virDevicePCIAddressPtr orig, + virDevicePCIAddressActor actor, void *opaque); virPCIDeviceListPtr virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev); -int virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr, - virPCIDeviceAddressPtr **iommuGroupDevices, +int virDevicePCIAddressGetIOMMUGroupAddresses(virDevicePCIAddressPtr devAddr, + virDevicePCIAddressPtr **iommuGroupDevices, size_t *nIommuGroupDevices); -int virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr); +int virDevicePCIAddressGetIOMMUGroupNum(virDevicePCIAddressPtr addr); char *virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev); int virPCIDeviceIsAssignable(virPCIDevicePtr dev, @@ -185,10 +186,10 @@ int virPCIDeviceIsAssignable(virPCIDevicePtr dev, int virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher); int virPCIGetPhysicalFunction(const char *vf_sysfs_path, - virPCIDeviceAddressPtr *pf); + virDevicePCIAddressPtr *pf); int virPCIGetVirtualFunctions(const char *sysfs_path, - virPCIDeviceAddressPtr **virtual_functions, + virDevicePCIAddressPtr **virtual_functions, size_t *num_virtual_functions, unsigned int *max_virtual_functions); @@ -198,7 +199,7 @@ int virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, const char *vf_sysfs_device_link, int *vf_index); -int virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, +int virDevicePCIAddressGetSysfsFile(virDevicePCIAddressPtr addr, char **pci_sysfs_device_link); int virPCIGetNetName(char *device_link_sysfs_path, char **netname); @@ -214,7 +215,7 @@ int virPCIGetAddrString(unsigned int domain, char **pciConfigAddr) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; -int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); +int virDevicePCIAddressParse(char *address, virDevicePCIAddressPtr bdf); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, char **pfname, int *vf_index); -- 2.8.1
All sub-PCI capabilities should be next to each other for clarity. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 22 +++++++++++----------- src/conf/node_device_conf.c | 9 ++++----- .../pci_0000_00_1c_0_header_type.xml | 2 +- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index d9375130487c..6b9b54298b21 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -149,6 +149,17 @@ </optional> <optional> + <element name='capability'> + <attribute name='type'> + <choice> + <value>pci-bridge</value> + <value>cardbus-bridge</value> + </choice> + </attribute> + </element> + </optional> + + <optional> <element name='iommuGroup'> <attribute name='number'> <ref name='unsignedInt'/> @@ -170,17 +181,6 @@ </optional> <optional> - <element name='capability'> - <attribute name='type'> - <choice> - <value>pci-bridge</value> - <value>cardbus-bridge</value> - </choice> - </attribute> - </element> - </optional> - - <optional> <element name='pci-express'> <zeroOrMore> <element name='link'> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 3e9c821762eb..b6e2f82727d0 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -383,6 +383,10 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferAddLit(&buf, "</capability>\n"); } } + if (data->pci_dev.hdrType) { + virBufferAsprintf(&buf, "<capability type='%s'/>\n", + virPCIHeaderTypeToString(data->pci_dev.hdrType)); + } if (data->pci_dev.nIommuGroupDevices) { virBufferAsprintf(&buf, "<iommuGroup number='%d'>\n", data->pci_dev.iommuGroupNumber); @@ -403,11 +407,6 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferAsprintf(&buf, "<numa node='%d'/>\n", data->pci_dev.numa_node); - if (data->pci_dev.hdrType) { - virBufferAsprintf(&buf, "<capability type='%s'/>\n", - virPCIHeaderTypeToString(data->pci_dev.hdrType)); - } - if (data->pci_dev.flags & VIR_NODE_DEV_CAP_FLAG_PCIE) virPCIEDeviceInfoFormat(&buf, data->pci_dev.pci_express); break; diff --git a/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml b/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml index dea5f05237ff..c1be9f7d9cc0 100644 --- a/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml +++ b/tests/nodedevschemadata/pci_0000_00_1c_0_header_type.xml @@ -8,10 +8,10 @@ <function>0</function> <product id='0x8c10'>8 Series/C220 Series Chipset Family PCI Express Root Port #1</product> <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='pci-bridge'/> <iommuGroup number='8'> <address domain='0x0000' bus='0x00' slot='0x1c' function='0x0'/> </iommuGroup> - <capability type='pci-bridge'/> <pci-express> <link validity='cap' port='1' speed='5' width='1'/> <link validity='sta' speed='2.5' width='1'/> -- 2.8.1
There were few things done in the nodedev code but we were lacking tests for it. And because of that we missed that the schema was not updated either. Fix the schema and add various test files to show the schema is correct. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/nodedev.rng | 7 ++++- tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml | 23 +++++++++++++++++ .../pci_0000_02_10_7_sriov_pf_vfs_all.xml | 29 +++++++++++++++++++++ ...i_0000_02_10_7_sriov_pf_vfs_all_header_type.xml | 30 ++++++++++++++++++++++ .../pci_0000_02_10_7_sriov_vfs.xml | 26 +++++++++++++++++++ .../pci_0000_02_10_7_sriov_zero_vfs_max_count.xml | 21 +++++++++++++++ 6 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all_header_type.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_vfs.xml create mode 100644 tests/nodedevschemadata/pci_0000_02_10_7_sriov_zero_vfs_max_count.xml diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 6b9b54298b21..93a88d883056 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -143,8 +143,13 @@ <value>virt_functions</value> </attribute> <optional> - <ref name='address'/> + <attribute name='maxCount'> + <ref name='unsignedInt'/> + </attribute> </optional> + <zeroOrMore> + <ref name='address'/> + </zeroOrMore> </element> </optional> diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml new file mode 100644 index 000000000000..8f243b4d6119 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov.xml @@ -0,0 +1,23 @@ +<device> + <name>pci_0000_02_10_7</name> + <parent>pci_0000_00_04_0</parent> + <capability type='pci'> + <domain>0</domain> + <bus>2</bus> + <slot>16</slot> + <function>7</function> + <product id='0x10ca'>82576 Virtual Function</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='phys_function'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x1'/> + </capability> + <iommuGroup number='31'> + <address domain='0x0000' bus='0x02' slot='0x10' function='0x7'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='2.5' width='4'/> + <link validity='sta' width='0'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml new file mode 100644 index 000000000000..9e8dace020a1 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml @@ -0,0 +1,29 @@ +<device> + <name>pci_0000_02_10_7</name> + <parent>pci_0000_00_04_0</parent> + <capability type='pci'> + <domain>0</domain> + <bus>2</bus> + <slot>16</slot> + <function>7</function> + <product id='0x10ca'>82576 Virtual Function</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='phys_function'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x1'/> + </capability> + <capability type='virt_functions' maxCount='7'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x2'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x3'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x4'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x5'/> + </capability> + <iommuGroup number='31'> + <address domain='0x0000' bus='0x02' slot='0x10' function='0x7'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='2.5' width='4'/> + <link validity='sta' width='0'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all_header_type.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all_header_type.xml new file mode 100644 index 000000000000..4e6323a3ecdd --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all_header_type.xml @@ -0,0 +1,30 @@ +<device> + <name>pci_0000_02_10_7</name> + <parent>pci_0000_00_04_0</parent> + <capability type='pci'> + <domain>0</domain> + <bus>2</bus> + <slot>16</slot> + <function>7</function> + <product id='0x10ca'>82576 Virtual Function</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='phys_function'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x1'/> + </capability> + <capability type='virt_functions' maxCount='7'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x2'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x3'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x4'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x5'/> + </capability> + <capability type='pci-bridge'/> + <iommuGroup number='31'> + <address domain='0x0000' bus='0x02' slot='0x10' function='0x7'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='2.5' width='4'/> + <link validity='sta' width='0'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_vfs.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_vfs.xml new file mode 100644 index 000000000000..355eaaad97d4 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_vfs.xml @@ -0,0 +1,26 @@ +<device> + <name>pci_0000_02_10_7</name> + <parent>pci_0000_00_04_0</parent> + <capability type='pci'> + <domain>0</domain> + <bus>2</bus> + <slot>16</slot> + <function>7</function> + <product id='0x10ca'>82576 Virtual Function</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='virt_functions'> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x2'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x3'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x4'/> + <address domain='0x0000' bus='0x02' slot='0x00' function='0x5'/> + </capability> + <iommuGroup number='31'> + <address domain='0x0000' bus='0x02' slot='0x10' function='0x7'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='2.5' width='4'/> + <link validity='sta' width='0'/> + </pci-express> + </capability> +</device> diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_zero_vfs_max_count.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_zero_vfs_max_count.xml new file mode 100644 index 000000000000..e9eb122bfec9 --- /dev/null +++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_zero_vfs_max_count.xml @@ -0,0 +1,21 @@ +<device> + <name>pci_0000_02_10_7</name> + <parent>pci_0000_00_04_0</parent> + <capability type='pci'> + <domain>0</domain> + <bus>2</bus> + <slot>16</slot> + <function>7</function> + <product id='0x10ca'>82576 Virtual Function</product> + <vendor id='0x8086'>Intel Corporation</vendor> + <capability type='virt_functions' maxCount='3'/> + <iommuGroup number='31'> + <address domain='0x0000' bus='0x02' slot='0x10' function='0x7'/> + </iommuGroup> + <numa node='0'/> + <pci-express> + <link validity='cap' port='0' speed='2.5' width='4'/> + <link validity='sta' width='0'/> + </pci-express> + </capability> +</device> -- 2.8.1
We were lacking tests that are checking for the completeness of our nodedev XMLs and also whether we output properly formatted ones. This patch adds parsing for the capability elements inside the <capability type='pci'> element. Also bunch of tests are added to show everything works properly. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/node_device_conf.c | 98 +++++++++++++++++++++++++++++++++++++++++++++ tests/nodedevxml2xmltest.c | 7 ++++ 2 files changed, 105 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b6e2f82727d0..5a41f7e0d2cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1269,15 +1269,103 @@ virPCIEDeviceInfoParseXML(xmlXPathContextPtr ctxt, static int +virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virNodeDevCapDataPtr data) +{ + char *maxFuncsStr = virXMLPropString(node, "maxCount"); + char *type = virXMLPropString(node, "type"); + xmlNodePtr *addresses = NULL; + xmlNodePtr orignode = ctxt->node; + int ret = -1; + size_t i = 0; + + ctxt->node = node; + + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability type")); + goto out; + } + + if (STREQ(type, "phys_function")) { + xmlNodePtr address = virXPathNode("./address[1]", ctxt); + + if (VIR_ALLOC(data->pci_dev.physical_function) < 0) + goto out; + + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; + + if (!address) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing address in 'phys_function' capability")); + goto out; + } + + if (virDevicePCIAddressParseXML(address, + data->pci_dev.physical_function) < 0) + goto out; + } else if (STREQ(type, "virt_functions")) { + int naddresses = virXPathNodeSet("./address", ctxt, &addresses); + + if (maxFuncsStr && + virStrToLong_uip(maxFuncsStr, NULL, 10, + &data->pci_dev.max_virtual_functions) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Malformed 'maxCount' parameter")); + goto out; + } + + if (VIR_ALLOC_N(data->pci_dev.virtual_functions, naddresses) < 0) + goto out; + + for (i = 0; i < naddresses; i++) { + virDevicePCIAddressPtr addr = NULL; + + if (VIR_ALLOC(addr) < 0) + goto out; + + if (virDevicePCIAddressParseXML(addresses[i], addr) < 0) { + VIR_FREE(addr); + goto out; + } + + if (VIR_APPEND_ELEMENT(data->pci_dev.virtual_functions, + data->pci_dev.num_virtual_functions, + addr) < 0) + goto out; + } + + data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; + } else { + int hdrType = virPCIHeaderTypeFromString(type); + + if (hdrType > 0 && !data->pci_dev.hdrType) + data->pci_dev.hdrType = hdrType; + } + + ret = 0; + out: + VIR_FREE(addresses); + VIR_FREE(maxFuncsStr); + VIR_FREE(type); + ctxt->node = orignode; + return ret; +} + + +static int virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, virNodeDeviceDefPtr def, xmlNodePtr node, virNodeDevCapDataPtr data) { xmlNodePtr orignode, iommuGroupNode, pciExpress; + xmlNodePtr *nodes = NULL; + int n = 0; int ret = -1; virPCIEDeviceInfoPtr pci_express = NULL; char *tmp = NULL; + size_t i = 0; orignode = ctxt->node; ctxt->node = node; @@ -1321,6 +1409,15 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, data->pci_dev.vendor_name = virXPathString("string(./vendor[1])", ctxt); data->pci_dev.product_name = virXPathString("string(./product[1])", ctxt); + if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) + goto out; + + for (i = 0; i < n; i++) { + if (virNodeDevPCICapabilityParseXML(ctxt, nodes[i], data) < 0) + goto out; + } + VIR_FREE(nodes); + if ((iommuGroupNode = virXPathNode("./iommuGroup[1]", ctxt))) { if (virNodeDevCapPCIDevIommuGroupParseXML(ctxt, iommuGroupNode, data) < 0) { @@ -1349,6 +1446,7 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, ret = 0; out: + VIR_FREE(nodes); VIR_FREE(tmp); virPCIEDeviceInfoFree(pci_express); ctxt->node = orignode; diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 96041f50b9cd..0ed06fdff3e2 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -91,7 +91,14 @@ mymain(void) DO_TEST("usb_device_1d6b_1_0000_00_1d_0"); DO_TEST("pci_8086_4238_pcie_wireless"); DO_TEST("pci_8086_0c0c_snd_hda_intel"); + DO_TEST("pci_0000_00_02_0_header_type"); + DO_TEST("pci_0000_00_1c_0_header_type"); DO_TEST("scsi_target0_0_0"); + DO_TEST("pci_0000_02_10_7_sriov"); + DO_TEST("pci_0000_02_10_7_sriov_vfs"); + DO_TEST("pci_0000_02_10_7_sriov_zero_vfs_max_count"); + DO_TEST("pci_0000_02_10_7_sriov_pf_vfs_all"); + DO_TEST("pci_0000_02_10_7_sriov_pf_vfs_all_header_type"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.8.1
On Wed, 2016-04-13 at 16:37 +0200, Martin Kletzander wrote:
We were lacking tests that are checking for the completeness of our nodedev XMLs and also whether we output properly formatted ones. This patch adds parsing for the capability elements inside the <capability type='pci'> element. Also bunch of tests are added to show everything works properly.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/node_device_conf.c | 98 +++++++++++++++++++++++++++++++++++++++++++++ tests/nodedevxml2xmltest.c | 7 ++++ 2 files changed, 105 insertions(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b6e2f82727d0..5a41f7e0d2cc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1269,15 +1269,103 @@ virPCIEDeviceInfoParseXML(xmlXPathContextPtr ctxt,
static int +virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virNodeDevCapDataPtr data) +{ + char *maxFuncsStr = virXMLPropString(node, "maxCount");
maxCountStr?
+ char *type = virXMLPropString(node, "type");
typeStr?
@@ -1321,6 +1409,15 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt, data->pci_dev.vendor_name = virXPathString("string(./vendor[1])", ctxt); data->pci_dev.product_name = virXPathString("string(./product[1])", ctxt);
+ if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) + goto out; + + for (i = 0; i < n; i++) { + if (virNodeDevPCICapabilityParseXML(ctxt, nodes[i], data) < 0) + goto out; + } + VIR_FREE(nodes);
This shouldn't be necessary as you free the memory in the out path anyway. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2016-04-13 at 16:37 +0200, Martin Kletzander wrote:
v2: - Just a rebase - I did *not* use virPCIDeviceAddress wording instead as discussed in the v1 thread. That's because we have lot of functions working with virDevicePCIAddress named exactly after that and renaming those would be ugly IMHO.
Sorry, but I feel pretty strongly the other way around: if it's defined in virpci.h, it should be called virPCI*. virDevicePCIAddress is used a lot but AFAICT the number of functions whose name is derived from it is just six. Moreover, we don't have other virDevice*Address types (or even just virDevice*) to set a precedent, but we have a bunch of virPCI* stuff including virPCIDevice, which happens to have a virPCIDeviceAddress among its members. Bikeshedding, I know, but there you have it :) I'll look at the actual code changes tomorrow. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Apr 14, 2016 at 06:24:26PM +0200, Andrea Bolognani wrote:
On Wed, 2016-04-13 at 16:37 +0200, Martin Kletzander wrote:
v2: - Just a rebase - I did *not* use virPCIDeviceAddress wording instead as discussed in the v1 thread. That's because we have lot of functions working with virDevicePCIAddress named exactly after that and renaming those would be ugly IMHO.
Sorry, but I feel pretty strongly the other way around: if it's defined in virpci.h, it should be called virPCI*.
virDevicePCIAddress is used a lot but AFAICT the number of functions whose name is derived from it is just six.
Moreover, we don't have other virDevice*Address types (or even just virDevice*) to set a precedent, but we have a bunch of virPCI* stuff including virPCIDevice, which happens to have a virPCIDeviceAddress among its members.
Bikeshedding, I know, but there you have it :)
No problem, I'm not against discussing this. Feel free to try it out if you want. I actually should've posted something like 2a/4 and 2b/4, i.e. two versions of patches so that it's visible what's nice and what's not :) If we don't reach a conclusion, I might do that later.
I'll look at the actual code changes tomorrow.
Cheers.
-- Andrea Bolognani Software Engineer - Virtualization Team
On Fri, 2016-04-15 at 06:40 +0200, Martin Kletzander wrote:
On Thu, Apr 14, 2016 at 06:24:26PM +0200, Andrea Bolognani wrote:
On Wed, 2016-04-13 at 16:37 +0200, Martin Kletzander wrote:
v2: - Just a rebase - I did *not* use virPCIDeviceAddress wording instead as discussed in the v1 thread. That's because we have lot of functions working with virDevicePCIAddress named exactly after that and renaming those would be ugly IMHO.
Sorry, but I feel pretty strongly the other way around: if it's defined in virpci.h, it should be called virPCI*.
virDevicePCIAddress is used a lot but AFAICT the number of functions whose name is derived from it is just six.
Moreover, we don't have other virDevice*Address types (or even just virDevice*) to set a precedent, but we have a bunch of virPCI* stuff including virPCIDevice, which happens to have a virPCIDeviceAddress among its members.
Bikeshedding, I know, but there you have it :)
No problem, I'm not against discussing this. Feel free to try it out if you want. I actually should've posted something like 2a/4 and 2b/4, i.e. two versions of patches so that it's visible what's nice and what's not :) If we don't reach a conclusion, I might do that later.
ACK series with the stuff pointed out in 4/4 taken care of on the condition that you *do not* rename stuff from virPCIDeviceAddress to virDevicePCIAddress. If you're not okay with that condition we can discuss the matter further next week :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Martin Kletzander