[libvirt] [PATCH] virNetDevGetLinkInfo: Don't report link speed if NIC's down
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */ if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname, lnk->state = tmp_state; + /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) { + lnk->speed = 0; + ret = 0; + goto cleanup; + } + VIR_FREE(path); VIR_FREE(buf); @@ -1884,6 +1894,7 @@ virNetDevGetLinkInfo(const char *ifname, if (virFileReadAll(path, 1024, &buf) < 0) { /* Some devices doesn't report speed, in which case we get EINVAL */ if (errno == EINVAL) { + lnk->speed = 0; ret = 0; goto cleanup; } -- 1.8.5.5
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) { + lnk->speed = 0; + ret = 0; + goto cleanup; + } +
Pity that's not standardized, but what you did makes more sense than anything else. If the link is down, then no need to report it.
VIR_FREE(path); VIR_FREE(buf);
@@ -1884,6 +1894,7 @@ virNetDevGetLinkInfo(const char *ifname, if (virFileReadAll(path, 1024, &buf) < 0) { /* Some devices doesn't report speed, in which case we get EINVAL */ if (errno == EINVAL) { + lnk->speed = 0; ret = 0; goto cleanup; } -- 1.8.5.5
ACK, Martin
On 06/13/2014 10:10 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) {
Also for VIR_INTERFACE_LOWER_LAYER_DOWN (verified by looking at the speed reported by a macvtap device when its physdev is down). And I'm not sure how to get an interface into "NOT_PRESENT" or "DORMANT" state, but I would imagine that the speed should be 0 in those cases too. ACK with LOWER_LAYER_DOWN added (I won't insist on the others until/unless I see experimental evidence that they need it). BTW, thinking more about bridge devices - maybe they should be given state "up" if the device has been ifup'ed. In other words, in their case you could call the functional equivalent of if_is_active() in netcf (which does an SIOCGIFFLAGS ioctl and checks for the IFF_UP flag). (in any case, bridges should probably just always report a speed of 0).
On Fri, Jun 13, 2014 at 10:31:53AM +0300, Laine Stump wrote:
On 06/13/2014 10:10 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) {
Also for VIR_INTERFACE_LOWER_LAYER_DOWN (verified by looking at the speed reported by a macvtap device when its physdev is down). And I'm not sure how to get an interface into "NOT_PRESENT" or "DORMANT" state, but I would imagine that the speed should be 0 in those cases too.
I've seen many other states I have no idea how to achieve. Wouldn't it make more sense to report the speed only if the state is UP?
ACK with LOWER_LAYER_DOWN added (I won't insist on the others until/unless I see experimental evidence that they need it).
BTW, thinking more about bridge devices - maybe they should be given state "up" if the device has been ifup'ed. In other words, in their case you could call the functional equivalent of if_is_active() in netcf (which does an SIOCGIFFLAGS ioctl and checks for the IFF_UP flag). (in any case, bridges should probably just always report a speed of 0).
On 06/13/2014 10:50 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 10:31:53AM +0300, Laine Stump wrote:
On 06/13/2014 10:10 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) {
Also for VIR_INTERFACE_LOWER_LAYER_DOWN (verified by looking at the speed reported by a macvtap device when its physdev is down). And I'm not sure how to get an interface into "NOT_PRESENT" or "DORMANT" state, but I would imagine that the speed should be 0 in those cases too.
I've seen many other states I have no idea how to achieve. Wouldn't it make more sense to report the speed only if the state is UP?
That makes enough sense to me that I changed my netcf patch to do just that - it sets speed to 0 unless operstate is "up". Still open for debate though :-) I just sent the patch to netcf-devel@lists.fedorahosted.org Note that netcf will now report link state for interfaces that are a subordinate of another interface (e.g. the ethernets attached to a bridge or a bond, or a bond attached to a bridge). libvirt tosses those out, so I should probably do a patch to remedy that.
ACK with LOWER_LAYER_DOWN added (I won't insist on the others until/unless I see experimental evidence that they need it).
BTW, thinking more about bridge devices - maybe they should be given state "up" if the device has been ifup'ed.
I've decided against this in netcf, and instead simply omit the <link> element entirely if the interface type is bridge.
In other words, in their case you could call the functional equivalent of if_is_active() in netcf (which does an SIOCGIFFLAGS ioctl and checks for the IFF_UP flag). (in any case, bridges should probably just always report a speed of 0).
On 13.06.2014 11:23, Laine Stump wrote:
On 06/13/2014 10:50 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 10:31:53AM +0300, Laine Stump wrote:
On 06/13/2014 10:10 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) {
Also for VIR_INTERFACE_LOWER_LAYER_DOWN (verified by looking at the speed reported by a macvtap device when its physdev is down). And I'm not sure how to get an interface into "NOT_PRESENT" or "DORMANT" state, but I would imagine that the speed should be 0 in those cases too.
I've seen many other states I have no idea how to achieve. Wouldn't it make more sense to report the speed only if the state is UP?
That makes enough sense to me that I changed my netcf patch to do just that - it sets speed to 0 unless operstate is "up". Still open for debate though :-) I just sent the patch to netcf-devel@lists.fedorahosted.org
So I'm not applying this patch then and proposing a different one: https://www.redhat.com/archives/libvir-list/2014-June/msg00647.html Michal
On 13.06.2014 09:31, Laine Stump wrote:
On 06/13/2014 10:10 AM, Martin Kletzander wrote:
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 6f3a202..80ef572 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1843,7 +1843,7 @@ virNetDevGetLinkInfo(const char *ifname, char *buf = NULL; char *tmp; int tmp_state; - unsigned int tmp_speed; + unsigned int tmp_speed; /* virInterfaceState */
You probably wanted to put this comment next to the line with tmp_state and not tmp_speed.
if (virNetDevSysfsFile(&path, ifname, "operstate") < 0) goto cleanup; @@ -1875,6 +1875,16 @@ virNetDevGetLinkInfo(const char *ifname,
lnk->state = tmp_state;
+ /* Shortcut to avoid some kernel issues. If link is down (and possibly in + * other states too) several drivers report several values. While igb + * reports 65535, realtek goes with 10. To avoid muddying XML with insane + * values, don't report link speed */ + if (lnk->state == VIR_INTERFACE_STATE_DOWN) {
Also for VIR_INTERFACE_LOWER_LAYER_DOWN (verified by looking at the speed reported by a macvtap device when its physdev is down). And I'm not sure how to get an interface into "NOT_PRESENT" or "DORMANT" state, but I would imagine that the speed should be 0 in those cases too.
If you use a wireless network that requires you to type in password each time you sign in, the wlan is in dormant state from the time it associates with an AP till you type in password and WiFi lets you in.
ACK with LOWER_LAYER_DOWN added (I won't insist on the others until/unless I see experimental evidence that they need it).
Okay.
BTW, thinking more about bridge devices - maybe they should be given state "up" if the device has been ifup'ed. In other words, in their case you could call the functional equivalent of if_is_active() in netcf (which does an SIOCGIFFLAGS ioctl and checks for the IFF_UP flag). (in any case, bridges should probably just always report a speed of 0).
I'm trying to keep the state libvirt reports in sync with state that the kernel reports. Michal
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway.
Wow, that's really awful. I think that warrants a bug report against the kernelt too since such inconsistency can't be useful to anyone. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/13/2014 11:30 AM, Daniel P. Berrange wrote:
The kernel's more broken than one would think. Various drivers report various (usually spurious) values if the interface is down. 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 down anyway. Wow, that's really awful. I think that warrants a bug report against
On Fri, Jun 13, 2014 at 08:46:59AM +0200, Michal Privoznik wrote: the kernelt too since such inconsistency can't be useful to anyone.
This list shows just the variations I found on my own system: https://www.redhat.com/archives/libvir-list/2014-June/msg00594.html
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Martin Kletzander -
Michal Privoznik