[libvirt] [PATCH] virNetDevGetLinkInfo: Don't report link speed if NIC's down

[Michal Privoznik]

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 at 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