[libvirt] [PATCH] nodedev: Fix failing to parse PCI address for non-PCI network devices
Commit 8708ca01c added virNetDevSwitchdevFeature to check whether the NIC had Switchdev capabilities; however this causes errors for network devices whose address is not in PCI format, like qeth device whose address is 0.0.0800, when calling virPCIDeviceAddressParse. Change virReportError to VIR_DEBUG to avoid these two error messages occuring when starting the libvirtd service, just leave their callers to handle the return values respectively. Signed-off-by: Fei Li <fli@suse.com> --- src/util/virpci.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index fe57bef32..880d7baba 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s, ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + VIR_DEBUG("Failed to convert '%s' to unsigned int", s); return ret; } @@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) goto out; if (virPCIDeviceAddressParse(config_address, bdf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse PCI config address '%s'"), - config_address); + VIR_DEBUG("Failed to parse PCI config address '%s'", config_address); VIR_FREE(bdf); goto out; } -- 2.13.5
On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
Commit 8708ca01c added virNetDevSwitchdevFeature to check whether the NIC had Switchdev capabilities; however this causes errors for network devices whose address is not in PCI format, like qeth device whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
Change virReportError to VIR_DEBUG to avoid these two error messages occuring when starting the libvirtd service, just leave their callers to handle the return values respectively.
Signed-off-by: Fei Li <fli@suse.com> --- src/util/virpci.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index fe57bef32..880d7baba 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + VIR_DEBUG("Failed to convert '%s' to unsigned int", s); return ret; }
@@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) goto out;
if (virPCIDeviceAddressParse(config_address, bdf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse PCI config address '%s'"), - config_address); + VIR_DEBUG("Failed to parse PCI config address '%s'", config_address); VIR_FREE(bdf); goto out; }
I am not familiar with switchdev feature, but this is certainly not the correct fix, errors do have their meaning and by simply mangling the log level of messages to suit your use case is not the way. Again, I'm far from an expert in this area, but IMHO you should special case handling of devices supporting this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create another one if suitable/appropriate in this case and skip parsing the PCI address completely. Erik
On 01/03/2018 01:40 AM, Erik Skultety wrote:
On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
Commit 8708ca01c added virNetDevSwitchdevFeature to check whether the NIC had Switchdev capabilities; however this causes errors for network devices whose address is not in PCI format, like qeth device whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
Change virReportError to VIR_DEBUG to avoid these two error messages occuring when starting the libvirtd service, just leave their callers to handle the return values respectively.
Signed-off-by: Fei Li <fli@suse.com> --- src/util/virpci.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index fe57bef32..880d7baba 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + VIR_DEBUG("Failed to convert '%s' to unsigned int", s); return ret; }
@@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) goto out;
if (virPCIDeviceAddressParse(config_address, bdf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse PCI config address '%s'"), - config_address); + VIR_DEBUG("Failed to parse PCI config address '%s'", config_address); VIR_FREE(bdf); goto out; }
I am not familiar with switchdev feature, but this is certainly not the correct fix, errors do have their meaning and by simply mangling the log level of messages to suit your use case is not the way.
I'll suggested this change when Fei and I discussed this bug off list. I'm far from an expert on this code as well, but I made the suggestions since some callers of virPCIGetDeviceAddressFromSysfsLink() report their own errors when it returns NULL and others treat a NULL return value as non-fatal. E.g. the virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->virPCIGetDeviceAddressFromSysfsLink() call chain. In addition, virPCIGetDeviceAddressFromSysfsLink() already has if (!virFileExists(device_link)) { VIR_DEBUG("'%s' does not exist", device_link); return NULL; } If changing error reporting to debug was acceptable, Fei's patch would need amended to change one more instance of virReportError().
Again, I'm far from an expert in this area, but IMHO you should special case handling of devices supporting this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create another one if suitable/appropriate in this case and skip parsing the PCI address completely.
IIUC, we are trying to determine if the interface supports VIR_NET_DEV_FEAT_SWITCHDEV in virNetDevSwitchdevFeature(), and part of that is checking if the interface is backed by a PCI device. But like you, I'm not familiar with this code and may be missing something. Adding the author of the feature to cc for suggestions. Regards, Jim
You're correct Jim, virNetDevSwitchdevFeature determines if the interface supports switchdev mode (hw offloading). We are using virNetDevGetPCIDevice to determine the interface's PF pci address in order to query the nic's capabilities using devlink. I'm not that familiar with the rest of the use cases for virNetDevGetPCIDevice but for us null return value tells us the interface doesn't support switchdev. -----Original Message----- From: Jim Fehlig [mailto:jfehlig@suse.com] Sent: Wednesday, January 03, 2018 7:42 PM To: Erik Skultety <eskultet@redhat.com>; Fei Li <fli@suse.com> Cc: libvir-list@redhat.com; Edan David <edand@mellanox.com> Subject: Re: [libvirt] [PATCH] nodedev: Fix failing to parse PCI address for non-PCI network devices On 01/03/2018 01:40 AM, Erik Skultety wrote:
On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
Commit 8708ca01c added virNetDevSwitchdevFeature to check whether the NIC had Switchdev capabilities; however this causes errors for network devices whose address is not in PCI format, like qeth device whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
Change virReportError to VIR_DEBUG to avoid these two error messages occuring when starting the libvirtd service, just leave their callers to handle the return values respectively.
Signed-off-by: Fei Li <fli@suse.com> --- src/util/virpci.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index fe57bef32..880d7baba 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + VIR_DEBUG("Failed to convert '%s' to unsigned int", s); return ret; }
@@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) goto out;
if (virPCIDeviceAddressParse(config_address, bdf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse PCI config address '%s'"), - config_address); + VIR_DEBUG("Failed to parse PCI config address '%s'", + config_address); VIR_FREE(bdf); goto out; }
I am not familiar with switchdev feature, but this is certainly not the correct fix, errors do have their meaning and by simply mangling the log level of messages to suit your use case is not the way.
I'll suggested this change when Fei and I discussed this bug off list. I'm far from an expert on this code as well, but I made the suggestions since some callers of virPCIGetDeviceAddressFromSysfsLink() report their own errors when it returns NULL and others treat a NULL return value as non-fatal. E.g. the virNetDevSwitchdevFeature()->virNetDevGetPCIDevice()->virPCIGetDeviceAddressFromSysfsLink() call chain. In addition, virPCIGetDeviceAddressFromSysfsLink() already has if (!virFileExists(device_link)) { VIR_DEBUG("'%s' does not exist", device_link); return NULL; } If changing error reporting to debug was acceptable, Fei's patch would need amended to change one more instance of virReportError().
Again, I'm far from an expert in this area, but IMHO you should special case handling of devices supporting this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create another one if suitable/appropriate in this case and skip parsing the PCI address completely.
IIUC, we are trying to determine if the interface supports VIR_NET_DEV_FEAT_SWITCHDEV in virNetDevSwitchdevFeature(), and part of that is checking if the interface is backed by a PCI device. But like you, I'm not familiar with this code and may be missing something. Adding the author of the feature to cc for suggestions. Regards, Jim
On 01/03/2018 01:40 AM, Erik Skultety wrote:
On Fri, Dec 22, 2017 at 01:05:26PM +0800, Fei Li wrote:
Commit 8708ca01c added virNetDevSwitchdevFeature to check whether the NIC had Switchdev capabilities; however this causes errors for network devices whose address is not in PCI format, like qeth device whose address is 0.0.0800, when calling virPCIDeviceAddressParse.
Change virReportError to VIR_DEBUG to avoid these two error messages occuring when starting the libvirtd service, just leave their callers to handle the return values respectively.
Signed-off-by: Fei Li <fli@suse.com> --- src/util/virpci.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index fe57bef32..880d7baba 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2561,7 +2561,7 @@ logStrToLong_ui(char const *s,
ret = virStrToLong_ui(s, end_ptr, base, result); if (ret != 0) - VIR_ERROR(_("Failed to convert '%s' to unsigned int"), s); + VIR_DEBUG("Failed to convert '%s' to unsigned int", s); return ret; }
@@ -2638,9 +2638,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) goto out;
if (virPCIDeviceAddressParse(config_address, bdf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse PCI config address '%s'"), - config_address); + VIR_DEBUG("Failed to parse PCI config address '%s'", config_address); VIR_FREE(bdf); goto out; }
I am not familiar with switchdev feature, but this is certainly not the correct fix, errors do have their meaning and by simply mangling the log level of messages to suit your use case is not the way. Again, I'm far from an expert in this area, but IMHO you should special case handling of devices supporting this feature by, e.g. using the VIR_NET_DEV_FEAT_SWITCHDEV enum or create another one if suitable/appropriate in this case and skip parsing the PCI address completely.
I've sent a patch that takes a slightly different approach to solving this problem https://www.redhat.com/archives/libvir-list/2018-January/msg00233.html Regards, Jim
participants (4)
-
Edan David -
Erik Skultety -
Fei Li -
Jim Fehlig