[libvirt] [PATCH 2/4] interface_backend_udev: Implement link speed & state

[Michal Privoznik]

On 04.06.2014 23:40, Daniel P. Berrange wrote:
> On Tue, Jun 03, 2014 at 02:22:10PM +0200, Michal Privoznik wrote:
>> In previous commit the interface XML is prepared for exporting
>> information on NIC link speed and state. This commit implement
>> actual logic for getting such info and writing it into XML.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>
>> Notes:
>>      For the kernel issue I've posted patch here:
>>
>>      http://patchwork.ozlabs.org/patch/354928/
>>
>>      But it wasn't accepted as developers are afraid of breaking backward
>>      compatibility. Which is weird as in 2.6.X the kernel was reporting -1
>>      instead of unsigned -1.
>>
>>   src/interface/interface_backend_udev.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
>> index c5353ea..80ba93e 100644
>> --- a/src/interface/interface_backend_udev.c
>> +++ b/src/interface/interface_backend_udev.c
>> @@ -1036,6 +1036,8 @@ udevGetIfaceDef(struct udev *udev, const char *name)
>>       const char *mtu_str;
>>       char *vlan_parent_dev = NULL;
>>       const char *devtype;
>> +    const char *operstate;
>> +    const char *link_speed;
>>
>>       /* Allocate our interface definition structure */
>>       if (VIR_ALLOC(ifacedef) < 0)
>> @@ -1059,6 +1061,31 @@ udevGetIfaceDef(struct udev *udev, const char *name)
>>                      udev_device_get_sysattr_value(dev, "address")) < 0)
>>           goto error;
>>
>> +    operstate = udev_device_get_sysattr_value(dev, "operstate");
>> +    if ((ifacedef->lnk.state = virInterfaceStateTypeFromString(operstate)) <= 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Unable to parse operstate: %s"),
>> +                       operstate);
>> +        goto error;
>> +    }
>> +
>> +    link_speed = udev_device_get_sysattr_value(dev, "speed");
>> +    if (link_speed) {
>> +        if (virStrToLong_ul(link_speed, NULL, 10, &ifacedef->lnk.speed) < 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Unable to parse speed: %s"),
>> +                           link_speed);
>> +            goto error;
>> +        }
>> +
>> +        /* 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. */
>> +        if ((int) ifacedef->lnk.speed == -1)
>> +            ifacedef->lnk.speed = 0;
>> +    }
>
> This block of code is duplicated in two other places in your series.
> I think it'd be worth having a function in virnetdev
>
>    virNetDevGetLinkInfo(const char *ifname, int *speed, int *state)

Yes and no. One place that adds these lines is in netcf backend. This is 
probably not going to be applied anyway since netcf is gonna expose the 
info itself. That leaves us with the second place, which is udev backend 
to node_device driver. So the function can be:

udevNetDevGetLinkInfo(struct udev_device *device,
                       int *speed, int *state);

Okay, I'll rework and repost. I'm not sure if it makes any sense to push 
the 1/4 now. Does it?

Michal