[netcf-devel] [PATCH] report link state/speed in interface status

Laine Stump laine at laine.org
Wed Jun 18 12:33:59 UTC 2014


On 06/17/2014 11:46 PM, Eric Blake wrote:
> On 06/13/2014 03:19 AM, Laine Stump wrote:
>> This patch adds reporting of link state and speed for appropriate
>> types of interfaces. It is added to the interface status as a single
>> element:
>>
>>   <link state='up|down|whatever' speed='nn'/>
>>
>> (the speed is in Mbits/sec, i.e. exactly what is reported by the
>> kernel). The information come from the following locations in sysfs:
>>
>>  state - /sys/class/net/$ifname/operstate
>>  speed - /sys/class/net/$ifname/speed
>>
>> Note that speed will be set to 0 if operstate is anything but
>> "up".
> Good, especially given how widely varying the speed numbers are for
> interfaces that are not up.
>
>> Also, this element is not added if an interface is of type
>> "bridge" (although any interfaces attached to the bridge, and reported
>> as subordinate interfaces in the same xml, *will* get a <link>
>> element.)
>>
>> An example - this is a bridge device which has one bond attached; the
>> bond is comprised of two ethernets:
>>
>>   <interface name="br0" type="bridge">
>>     <bridge>
>>       <interface name="bond0" type="bond">
>>         <link state="up" speed="1000"/>
>>         <bond>
>>           <interface name="p13p2_5" type="ethernet">
>>             <link state="up" speed="1000"/>
>>             <mac address="ea:8b:8d:18:67:fe"/>
>>           </interface>
>>           <interface name="p13p2_6" type="ethernet">
>>             <link state="up" speed="1000"/>
> Is it possible to bond together two interfaces of different speeds; and
> if so, what is the speed for the bond?

EDONTKNOW :-)

>
>> (note that in this case the bond reports a speed of 1000, although in
>> some cases I have seen it report "2000". This is an issue of the
>> kernel driver - netcf merely reports whatever the kernel has said.)
> That may also be a factor of whether the bond can use both interfaces at
> once to double throughput, or whether the bond is tied to using one or
> the other interface for the same overall speed.


Except that I didn't change the configuration of that bond at all during
this testing :-/


>   But yeah, relying on
> the kernel numbers makes sense.

Yep. The kernel may be providing bad information at times now, but
trying to second-guess it can only lead to problems later (or even right
away - e.g. if we decided to report the bond speed as the sum of all
ethernets attached to it, not understanding that some bonds are a "use
only one at a time, and reserving the others for failover")


>
>
>> +    if (!strcmp(state, "up")) {
>> +        /* When the link state is "down", different drivers report a
>> +         * different value for speed. In one local sample, the
>> +         * following were seen: "10", "65535", "4294967295". Since the
>> +         * effective speed is "0", just change it to that whenever the
>> +         * link state is not "up".
>> +         */
> Does this comment belong better in the 'else' branch?

Yes, good catch - the result of changing around the logic but not
keeping the comments up to date with those changes.

>
>> +        xasprintf(&path, "/sys/class/net/%s/speed", ifname);
>> +        ERR_NOMEM(path == NULL, ncf);
>> +        speed = read_file(path, &length);
>> +        if (!speed && errno == EINVAL) {
>> +            /* attempts to read $ifname/speed result in EINVAL if the
>> +             * interface is ifconfiged down (which isn't exactly the
>> +             * same as an operstate of "down").
>> +             */
>> +            speed = strdup("0");
>> +            ERR_NOMEM(!speed, ncf);
>> +        }
>> +        ERR_THROW_STRERROR(!speed, ncf, EFILE, "Failed to read %s", path);
>> +        if ((nl = strchr(speed, '\n')))
>> +            *nl = 0;
>> +    } else {
>> +        FREE(speed);
>> +        speed = strdup("0");
>> +        ERR_NOMEM(!speed, ncf);
>> +    }
>> @@ -1061,6 +1129,7 @@ error:
>>      return;
>>  }
>>  
>> +
>>  void add_state_to_xml_doc(struct netcf_if *nif, xmlDocPtr doc) {
> Spurious whitespace change?
>
> Overall, looks reasonable to me.  ACK.

I'm adding a small bit to the xml schema and pushing with your other
points addressed.

Thanks!




More information about the netcf-devel mailing list