[libvirt] [PATCHv2] pci: properly handle out-of-order SRIOV virtual functions
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1025397 When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case. This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior. The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name). Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. --- Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message. (Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.) src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..f744c8b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return ret; } +static const char *virtfnPrefix = "virtfn"; + /* * Returns virtual functions of a physical function */ @@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + char **virtfnNames = NULL; + size_t nVirtfnNames = 0; VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path, while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName; - if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + } - VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames; - if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex; - if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + } + + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } + + if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } - (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) { + VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; } + + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); } + /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); + + /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path); + goto error; + } + } + + VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions); ret = 0; cleanup: + for (i = 0; i < nVirtfnNames; i++) + VIR_FREE(virtfnNames[i]); + VIR_FREE(virtfnNames); VIR_FREE(device_link); if (dir) closedir(dir); return ret; error: - if (*virtual_functions) { - for (i = 0; i < *num_virtual_functions; i++) - VIR_FREE((*virtual_functions)[i]); - VIR_FREE(*virtual_functions); - } + for (i = 0; i < *num_virtual_functions; i++) + VIR_FREE((*virtual_functions)[i]); + VIR_FREE(*virtual_functions); goto cleanup; } -- 1.8.3.1
On Thu, Nov 07, 2013 at 01:10:39PM +0200, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case.
This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. ---
Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-)
Disclaimer: I don't want this to sound rude (just that it does to me every time I read it), this is just a question that popped in my mind. Why don't we do it differently? If we have a while loop with a number starting from 1 and access(, F_OK)'ing '"virtfn%d", number' in that loop, we'll have all those VFs sorted. And if that's not nice enough, we should be able to read the number of VFs from the device's "/config" (in offset 0x10 [1]) and do a nice, clean for loop over already known filenames. Or am I missing something? I just Took that from "drivers/pci/iov.c", so this may be "Just Wrong". Martin [1] http://lxr.free-electrons.com/source/include/uapi/linux/pci_regs.h#L804
diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..f744c8b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return ret; }
+static const char *virtfnPrefix = "virtfn"; + /* * Returns virtual functions of a physical function */ @@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + char **virtfnNames = NULL; + size_t nVirtfnNames = 0;
VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + }
- VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames;
- if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex;
- if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + } + + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } + + if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + }
- (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) { + VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; } + + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); }
+ /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); + + /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path); + goto error; + } + } + + VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions); ret = 0;
cleanup: + for (i = 0; i < nVirtfnNames; i++) + VIR_FREE(virtfnNames[i]); + VIR_FREE(virtfnNames); VIR_FREE(device_link); if (dir) closedir(dir); return ret;
error: - if (*virtual_functions) { - for (i = 0; i < *num_virtual_functions; i++) - VIR_FREE((*virtual_functions)[i]); - VIR_FREE(*virtual_functions); - } + for (i = 0; i < *num_virtual_functions; i++) + VIR_FREE((*virtual_functions)[i]); + VIR_FREE(*virtual_functions); goto cleanup; }
-- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/07/2013 02:08 PM, Martin Kletzander wrote:
On Thu, Nov 07, 2013 at 01:10:39PM +0200, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case.
This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. ---
Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-)
Disclaimer: I don't want this to sound rude (just that it does to me every time I read it), this is just a question that popped in my mind.
Not at all. Alternatives are always welcome.
Why don't we do it differently? If we have a while loop with a number starting from 1 and access(, F_OK)'ing '"virtfn%d", number' in that loop, we'll have all those VFs sorted. And if that's not nice enough, we should be able to read the number of VFs from the device's "/config" (in offset 0x10 [1]) and do a nice, clean for loop over already known filenames.
Or am I missing something? I just Took that from "drivers/pci/iov.c", so this may be "Just Wrong".
No, that would work too. I was just looking at it from the inside rather than the outside, so was only thinking of how to fix the current code rather than how to rewrite it, and never considered that. My one concern would be if someone actually pushed a patch into the kernel similar to the one suggested by the original reporter of this problem (which put leading 0's on all the numbers in the virtfn filenames), but that seems unlikely, as it isn't scalable (what happens when you get > 100 VFs?). I like your idea enough to redo the patch using it. It may be tomorrow before I get it posted though.
On 11/07/2013 02:28 PM, Laine Stump wrote:
On 11/07/2013 02:08 PM, Martin Kletzander wrote:
On Thu, Nov 07, 2013 at 01:10:39PM +0200, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case.
This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. ---
Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-)
Disclaimer: I don't want this to sound rude (just that it does to me every time I read it), this is just a question that popped in my mind. Not at all. Alternatives are always welcome.
Why don't we do it differently? If we have a while loop with a number starting from 1 and access(, F_OK)'ing '"virtfn%d", number' in that loop, we'll have all those VFs sorted. And if that's not nice enough, we should be able to read the number of VFs from the device's "/config" (in offset 0x10 [1]) and do a nice, clean for loop over already known filenames.
Or am I missing something? I just Took that from "drivers/pci/iov.c", so this may be "Just Wrong". No, that would work too. I was just looking at it from the inside rather than the outside, so was only thinking of how to fix the current code rather than how to rewrite it, and never considered that.
My one concern would be if someone actually pushed a patch into the kernel similar to the one suggested by the original reporter of this problem (which put leading 0's on all the numbers in the virtfn filenames), but that seems unlikely, as it isn't scalable (what happens when you get > 100 VFs?).
I like your idea enough to redo the patch using it. It may be tomorrow before I get it posted though.
I've posted a patch based on this suggestion (2 patches actually, due to a prerequisite that changes the type of num_virtual_functions from unsigned int to size_t), and I think the results are very good - rather than adding new code, about 30 lines were removed. The result is simpler even than the original, and ends up with correct results.
On 11/07/2013 12:10 PM, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case.
This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. ---
Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..f744c8b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return ret; }
+static const char *virtfnPrefix = "virtfn"; + /* * Returns virtual functions of a physical function */ @@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + char **virtfnNames = NULL; + size_t nVirtfnNames = 0;
VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + }
Wouldn't it be easier to just count the entries beginning with "virtfn" and then go from 0 to n? (This would need a special return value for virPCIGetDeviceAddressFromSysfsLink if the link does not exist, if the entries in the directory can truly be sparse, or a virtfn-poorly-chosen-name appears)
- VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames;
- if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex;
- if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + }
This rejects "virtfn-poor-name-choice" (which is fine by me, it's really a poor choice).
+ + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } +
+ if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + }
Can there really be two entries in a directory with the same name?
- (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) {
This also ignores OOM errors.
+ VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; }
+ + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); }
+ /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); +
This treats contiguous missing entries at the end differently than the missing ones at the beginning, are the virtfns with larger indexes less important? :)
+ /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path);
In other words, virPCIGetDeviceAddressFromSysfsLink failed. If we really need to ignore entries that aren't a symlink with a PCI address, the array has to be sparse and we need to fix all the users :(
+ goto error; + } + } + + VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);
Jan
Hi forget this as approach works using kernel-patch. Correctly was mentioned that the approach using leading-zeroes was a try to fix something happened in other place. Haven't get intention to libvirt side after that, wrote untested patch for this symptom. It's is not complete - no more error checking's. But it should pre-select only "virtfn" entries to be handled in while-loop, and after that scan through them in increasing numerical order. Still, here it is : < /* routine to select only "virtfn" -entries */ < static int < virtfn_select(const struct dirent *entry) < { < return (strncmp(entry->d_name,"virtfn", 6) == 0) ? 1 : 0; < } < 2401a2395,2396
DIR *dir = NULL; struct dirent *entry = NULL;
2404,2406d2398 < struct dirent **namelist; < int entry_count = 0; < int current_entry = 0; 2414,2415c2406,2407 < struct stat sb; < if ((stat(sysfs_path, &sb) == -1) || ((sb.st_mode & S_IFMT) != S_IFDIR)) { ---
dir = opendir(sysfs_path); if (dir == NULL) {
2423,2425c2415,2416 < entry_count = scandir(sysfs_path, &namelist, virtfn_select, alphasort); < < while ( current_entry < entry_count ) { ---
while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) {
2428c2419 < if (virBuildPath(&device_link, sysfs_path, namelist[ current_entry ]->d_name ) == -1) { ---
if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
2432c2423 < current_entry ++; ---
2453a2445
}
2460,2462c2452,2453 < for ( i = 0; i < entry_count; i ++) < free( namelist[ i ] ); < free( namelist ); ---
if (dir) closedir(dir);
On Thu, Nov 7, 2013 at 2:32 PM, Ján Tomko <jtomko@redhat.com> wrote:
On 11/07/2013 12:10 PM, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1025397
When libvirt creates the list of an SRIOV Physical Function's (PF) Virtual Functions (VF), it assumes that the order of "virtfn*" links returned by readdir() from the PF's sysfs directory is already in the correct order. Experience has shown that this is not always the case.
This results in 1) incorrect assumptions made by consumers of the output of the virt_functions list of virsh nodedev-dumpxml, and 2) setting MAC address and vlan tag on the wrong VF (since libvirt uses netlink to set mac address and vlan tag, netlink requires the VF#, and the function virPCIGetVirtualFunctionIndex() returns the wrong index due to the improperly ordered VF list). See the bugzilla report for an example of this improper behavior.
The solution provided by this patch is for virPCIGetVirtualFunctions to first gather all the "virtfn*" names from the PFs sysfs directory, then allocate a virtual_functions array large enough to hold all entries, and finally to create a device entry for each virtfn* name and place it into the virtual_functions array at the proper index (based on interpreting the value following "virtfn" in the name).
Checks are introduced to ensure that 1) the virtfn list given in sysfs is not sparse (ie, there are no indexes larger than the length of the list), and that no two virtfn* entries decode to the same index. ---
Difference from V1: Based on jtomko's review, truncate trailing NULLs in virtual_functions array, and actually check for NULLs in the middle (a sparse array) and generate an error in that case, as we already claimed to do in the commit message.
(Sorry for the lack of inter-diff, but the only changes from V1 are the addition of the lines starting with "truncate any trailing nulls" and continuing until the next VIR_DEBUG.)
src/util/virpci.c | 115 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 25 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 148631f..f744c8b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2379,6 +2379,8 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return ret; }
+static const char *virtfnPrefix = "virtfn"; + /* * Returns virtual functions of a physical function */ @@ -2393,6 +2395,8 @@ virPCIGetVirtualFunctions(const char *sysfs_path, struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + char **virtfnNames = NULL; + size_t nVirtfnNames = 0;
VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2411,51 +2415,112 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + }
Wouldn't it be easier to just count the entries beginning with "virtfn" and then go from 0 to n? (This would need a special return value for virPCIGetDeviceAddressFromSysfsLink if the link does not exist, if the entries in the directory can truly be sparse, or a virtfn-poorly-chosen-name appears)
- VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames;
- if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex;
- if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + }
This rejects "virtfn-poor-name-choice" (which is fine by me, it's really a poor choice).
+ + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } +
+ if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + }
Can there really be two entries in a directory with the same name?
- (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) {
This also ignores OOM errors.
+ VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; }
+ + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); }
+ /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); +
This treats contiguous missing entries at the end differently than the missing ones at the beginning, are the virtfns with larger indexes less important? :)
+ /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path);
In other words, virPCIGetDeviceAddressFromSysfsLink failed. If we really need to ignore entries that aren't a symlink with a PCI address, the array has to be sparse and we need to fix all the users :(
+ goto error; + } + } + + VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);
Jan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- --------------------------------- Niilo Minkkinen niilo.minkkinen@iki.fi “Brittiläisen tutkimuksen mukaan pyöräily pidentää elinajan odotetta peräti 20 kertaa enemmän kuin onnettomuusriskin kohoaminen sitä lyhentää! (BMA, British Medical Association. Cycling: towards health & safety. Oxford University Press, 1992). ---------------------------------
On 11/07/2013 07:06 AM, Niilona wrote:
Hi forget this as approach works using kernel-patch. Correctly was mentioned that the approach using leading-zeroes was a try to fix something happened in other place. Haven't get intention to libvirt side after that, wrote untested patch for this symptom. It's is not complete - no more error checking's. But it should pre-select only "virtfn" entries to be handled in while-loop, and after that scan through them in increasing numerical order.
Still, here it is :
< /* routine to select only "virtfn" -entries */ < static int < virtfn_select(const struct dirent *entry) < { < return (strncmp(entry->d_name,"virtfn", 6) == 0) ? 1 : 0; < } < 2401a2395,2396
DIR *dir = NULL; struct dirent *entry = NULL;
Ouch. This is an ed-script diff, which is practically worthless if we don't know what version of the file to apply it to. Can you please resend as a proper context diff? Also, please don't top-post on technical lists. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Nov 7, 2013 at 5:38 PM, Eric Blake <eblake@redhat.com> wrote:
On 11/07/2013 07:06 AM, Niilona wrote:
Hi forget this as approach works using kernel-patch. Correctly was mentioned that the approach using leading-zeroes was a try to fix something happened in other place. Haven't get intention to libvirt side after that, wrote untested patch for this symptom. It's is not complete - no more error checking's. But it should pre-select only "virtfn" entries to be handled in while-loop, and after that scan through them in increasing numerical order.
Still, here it is :
< /* routine to select only "virtfn" -entries */ < static int < virtfn_select(const struct dirent *entry) < { < return (strncmp(entry->d_name,"virtfn", 6) == 0) ? 1 : 0; < } < 2401a2395,2396
DIR *dir = NULL; struct dirent *entry = NULL;
Ouch. This is an ed-script diff, which is practically worthless if we don't know what version of the file to apply it to. Can you please resend as a proper context diff? Also, please don't top-post on technical lists.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Sorry about behaving inappropriately. This is not a formal git-patch as download it on tar-package. Still hope the intention is clear : diff -rupN libvirt-1.1.3.org/libvirt-1.1.3/src/util/virpci.c libvirt-1.1.3/src/util/virpci.c --- libvirt-1.1.3.org/libvirt-1.1.3/src/util/virpci.c 2013-09-18 12:51:27.000000000 +0300 +++ libvirt-1.1.3/src/util/virpci.c 2013-10-25 13:56:47.703067652 +0300 @@ -2382,6 +2382,13 @@ virPCIGetPhysicalFunction(const char *vf return ret; } +/* routine to select only "virtfn" -entries */ +static int +virtfn_select(const struct dirent *entry) +{ + return (strncmp(entry->d_name,"virtfn", 6) == 0) ? 1 : 0; +} + /* * Returns virtual functions of a physical function */ @@ -2392,10 +2399,11 @@ virPCIGetVirtualFunctions(const char *sy { int ret = -1; size_t i; - DIR *dir = NULL; - struct dirent *entry = NULL; char *device_link = NULL; char errbuf[64]; + struct dirent **namelist; + int entry_count = 0; + int current_entry = 0; VIR_DEBUG("Attempting to get SR IOV virtual functions for device" "with sysfs path '%s'", sysfs_path); @@ -2403,8 +2411,8 @@ virPCIGetVirtualFunctions(const char *sy *virtual_functions = NULL; *num_virtual_functions = 0; - dir = opendir(sysfs_path); - if (dir == NULL) { + struct stat sb; + if ((stat(sysfs_path, &sb) == -1) || ((sb.st_mode & S_IFMT) != S_IFDIR)) { memset(errbuf, '\0', sizeof(errbuf)); virReportSystemError(errno, _("Failed to open dir '%s'"), @@ -2412,15 +2420,16 @@ virPCIGetVirtualFunctions(const char *sy return ret; } - while ((entry = readdir(dir))) { - if (STRPREFIX(entry->d_name, "virtfn")) { + entry_count = scandir(sysfs_path, &namelist, virtfn_select, alphasort); + + while ( current_entry < entry_count ) { virPCIDeviceAddress *config_addr = NULL; - if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { + if (virBuildPath(&device_link, sysfs_path, namelist[ current_entry ]->d_name ) == -1) { virReportOOMError(); goto error; } - + current_entry ++; VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions); @@ -2442,15 +2451,15 @@ virPCIGetVirtualFunctions(const char *sy (*virtual_functions)[*num_virtual_functions] = config_addr; (*num_virtual_functions)++; VIR_FREE(device_link); - } } ret = 0; cleanup: VIR_FREE(device_link); - if (dir) - closedir(dir); + for ( i = 0; i < entry_count; i ++) + free( namelist[ i ] ); + free( namelist ); return ret; error:
On 11/08/2013 08:47 AM, Niilona wrote:
@@ -2412,15 +2420,16 @@ virPCIGetVirtualFunctions(const char *sy return ret; }
- while ((entry = readdir(dir))) { - if (STRPREFIX(entry->d_name, "virtfn")) { + entry_count = scandir(sysfs_path, &namelist, virtfn_select, alphasort);
alphasort will make sure that the order is always wrong if there are more than 10 functions. Hopefully we can use versionsort thanks to gnulib.
+ + while ( current_entry < entry_count ) { virPCIDeviceAddress *config_addr = NULL;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { + if (virBuildPath(&device_link, sysfs_path, namelist[ current_entry ]->d_name ) == -1) { virReportOOMError(); goto error; } - + current_entry ++; VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);
On Fri, Nov 8, 2013 at 10:20 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 11/08/2013 08:47 AM, Niilona wrote:
@@ -2412,15 +2420,16 @@ virPCIGetVirtualFunctions(const char *sy return ret; }
- while ((entry = readdir(dir))) { - if (STRPREFIX(entry->d_name, "virtfn")) { + entry_count = scandir(sysfs_path, &namelist, virtfn_select, alphasort);
alphasort will make sure that the order is always wrong if there are more than 10 functions.
Hopefully we can use versionsort thanks to gnulib.
+ + while ( current_entry < entry_count ) { virPCIDeviceAddress *config_addr = NULL;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { + if (virBuildPath(&device_link, sysfs_path, namelist[ current_entry ]->d_name ) == -1) { virReportOOMError(); goto error; } - + current_entry ++; VIR_DEBUG("Number of virtual functions: %d", *num_virtual_functions);
true that alphasort breaks advantage, what's obtained using scandir(). versionsort is a good point.
On 11/07/2013 02:32 PM, Ján Tomko wrote:
On 11/07/2013 12:10 PM, Laine Stump wrote:
while ((entry = readdir(dir))) { if (STRPREFIX(entry->d_name, "virtfn")) { - virPCIDeviceAddress *config_addr = NULL; + char *tempName;
- if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) { - virReportOOMError(); + /* save these to sort into virtual_functions array */ + if (VIR_STRDUP(tempName, entry->d_name) < 0) + goto error; + if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) { + VIR_FREE(tempName); goto error; } + } + }
Wouldn't it be easier to just count the entries beginning with "virtfn" and then go from 0 to n?
Yes (although that has the problem you indicate below). Or just use Martin's idea of checking for existence of links starting with virtfn0 and increasing by one until there's a failure. I'd just been so wrapped up in micro-changing the existing code that I didn't look for a different solution.
(This would need a special return value for virPCIGetDeviceAddressFromSysfsLink if the link does not exist, if the entries in the directory can truly be sparse, or a virtfn-poorly-chosen-name appears)
- VIR_DEBUG("Number of virtual functions: %d", - *num_virtual_functions); + /* pre-allocate because we will be filling in out of order */ + if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0) + goto error; + *num_virtual_functions = nVirtfnNames;
- if (virPCIGetDeviceAddressFromSysfsLink(device_link, - &config_addr) != - SRIOV_FOUND) { - VIR_WARN("Failed to get SRIOV function from device " - "link '%s'", device_link); - VIR_FREE(device_link); - continue; - } + for (i = 0; i < nVirtfnNames; i++) { + virPCIDeviceAddress *config_addr = NULL; + unsigned int virtfnIndex;
- if (VIR_REALLOC_N(*virtual_functions, - *num_virtual_functions + 1) < 0) { - VIR_FREE(config_addr); - goto error; - } + if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) { + virReportOOMError(); + goto error; + } + + if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix), + 0, 10, &virtfnIndex) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid virtual function link name '%s' " + "in physical function directory '%s'"), + virtfnNames[i], sysfs_path); + goto error; + }
This rejects "virtfn-poor-name-choice" (which is fine by me, it's really a poor choice).
yeah, good point. Yet another reason this is a bad idea.
+ + if (virtfnIndex >= nVirtfnNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' larger than " + "total count of virtual functions %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } + + if ((*virtual_functions)[virtfnIndex]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Virtual function link name '%s' " + "generates duplicate index %zu " + "in physical function directory '%s'"), + virtfnNames[i], nVirtfnNames, sysfs_path); + goto error; + } Can there really be two entries in a directory with the same name?
No, but conceivably there could be, e.g., "virtfn01" and "virtfn1". It would indicate a serious problem in the kernel, but it's within the realm of possibility. So this was basically a sanity check.
- (*virtual_functions)[*num_virtual_functions] = config_addr; - (*num_virtual_functions)++; + if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) != + SRIOV_FOUND) {
This also ignores OOM errors.
yes, but in a non-dangerous manner. And there would eventually be an error exit because the array would end up being sparse.
+ VIR_WARN("Failed to get SRIOV function from device link '%s'", + device_link); VIR_FREE(device_link); + continue; } + + VIR_DEBUG("Found virtual function %d", virtfnIndex); + (*virtual_functions)[virtfnIndex] = config_addr; + VIR_FREE(device_link); }
+ /* truncate any trailing nulls due to files having names starting + * with "virtfn" which weren't actually a link to a virtual + * function. + */ + while (*num_virtual_functions && + !(*virtual_functions)[(*num_virtual_functions) - 1]) + (*num_virtual_functions)--; + + /* failure of realloc when shrinking is safe */ + ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions)); + This treats contiguous missing entries at the end differently than the missing ones at the beginning,
Exactly.
are the virtfns with larger indexes less important? :)
No. The logic behind this is that any extra entries at the end are due to filenames that start with "virtfn" but aren't actually a link to a virtual function at all (just a poorly chosen name for something else). The actual virtual functions will all be virtfn[0-n] and contiguous. So an empty entry at the end just indicates there were some items in the directory that weren't virtual function links, while an empty entry in the middle indicates that there was a problem resolving the link to an actual virtual function. Anyway, the fact that I need to explain this separately is more evidence that I should just do it in the simpler manner that Martin suggested, which I'm about to do now.
+ /* if there are any NULLs left in the array, fail and log an error + * - the virtual function array cannot be sparse. + */ + for (i = 0; i < *num_virtual_functions; i++) { + if (!(*virtual_functions)[*num_virtual_functions]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing virtual function link for VF %zu " + "in physical function directory '%s'"), + i, sysfs_path); In other words, virPCIGetDeviceAddressFromSysfsLink failed. If we really need to ignore entries that aren't a symlink with a PCI address, the array has to be sparse and we need to fix all the users :(
No, I believe the kernel will only create a contiguous array of virtual functions, so we don't need to support a sparse array.
participants (5)
-
Eric Blake -
Ján Tomko -
Laine Stump -
Martin Kletzander -
Niilona