[libvirt] [PATCH] virNetDevGetLinkInfo: Don't report link speed if NIC's not up
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is in other state than 'up' . While on some we experience -EINVAL when read()-ing the speed sysfs file, with other drivers we might get anything from 0 to UINT_MAX. If that's the case it's better to not report link speed. Well, the interface is not up anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..a551f98 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname, lnk->state = tmp_state; + /* Shortcut to avoid some kernel issues. If link is not up several drivers + * report several misleading values. While igb reports 65535, realtek goes + * with 10. To avoid muddying XML with insane values, don't report link + * speed if that's the case. */ + if (lnk->state != VIR_INTERFACE_STATE_UP) { + lnk->speed = 0; + ret = 0; + goto cleanup; + } + VIR_FREE(path); VIR_FREE(buf); @@ -1901,11 +1911,7 @@ virNetDevGetLinkInfo(const char *ifname, goto cleanup; } - /* Workaround broken kernel API. If the link is unplugged then - * depending on the NIC driver, link speed can be reported as -1. - * However, the value is printed out as unsigned integer instead of - * signed one. Terrifying but true. */ - lnk->speed = (int) tmp_speed == -1 ? 0 : tmp_speed; + lnk->speed = tmp_speed; ret = 0; cleanup: -- 1.8.5.5
On 06/13/2014 12:34 PM, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is in other state than 'up' . While on some we experience -EINVAL when read()-ing the speed sysfs file, with other drivers we might get anything from 0 to UINT_MAX. If that's the case it's better to not report link speed. Well, the interface is not up anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..a551f98 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is not up several drivers + * report several misleading values. While igb reports 65535, realtek goes + * with 10. To avoid muddying XML with insane values, don't report link + * speed if that's the case. */ + if (lnk->state != VIR_INTERFACE_STATE_UP) { + lnk->speed = 0; + ret = 0; + goto cleanup; + } + VIR_FREE(path); VIR_FREE(buf);
@@ -1901,11 +1911,7 @@ virNetDevGetLinkInfo(const char *ifname, goto cleanup; }
- /* Workaround broken kernel API. If the link is unplugged then - * depending on the NIC driver, link speed can be reported as -1. - * However, the value is printed out as unsigned integer instead of - * signed one. Terrifying but true. */ - lnk->speed = (int) tmp_speed == -1 ? 0 : tmp_speed; + lnk->speed = tmp_speed;
ret = 0; cleanup:
ACK from me. Might want to give a short time for differing opinions (or just push this and change as required up until the next release :-)
On 13.06.2014 12:25, Laine Stump wrote:
On 06/13/2014 12:34 PM, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is in other state than 'up' . While on some we experience -EINVAL when read()-ing the speed sysfs file, with other drivers we might get anything from 0 to UINT_MAX. If that's the case it's better to not report link speed. Well, the interface is not up anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..a551f98 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is not up several drivers + * report several misleading values. While igb reports 65535, realtek goes + * with 10. To avoid muddying XML with insane values, don't report link + * speed if that's the case. */ + if (lnk->state != VIR_INTERFACE_STATE_UP) { + lnk->speed = 0; + ret = 0; + goto cleanup; + } + VIR_FREE(path); VIR_FREE(buf);
@@ -1901,11 +1911,7 @@ virNetDevGetLinkInfo(const char *ifname, goto cleanup; }
- /* Workaround broken kernel API. If the link is unplugged then - * depending on the NIC driver, link speed can be reported as -1. - * However, the value is printed out as unsigned integer instead of - * signed one. Terrifying but true. */ - lnk->speed = (int) tmp_speed == -1 ? 0 : tmp_speed; + lnk->speed = tmp_speed;
ret = 0; cleanup:
ACK from me. Might want to give a short time for differing opinions (or just push this and change as required up until the next release :-)
Right. I've pushed this one as it enables testing for every one of us running libvirt from git instead of having to manually apply the patch. Michal
participants (2)
-
Laine Stump -
Michal Privoznik