[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