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". 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"/> <mac address="ea:8b:8d:18:67:fe"/> </interface> </bond> </interface> </bridge> </interface>
(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.) --- src/dutil_linux.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/src/dutil_linux.c b/src/dutil_linux.c index 2db166a..62e0e72 100644 --- a/src/dutil_linux.c +++ b/src/dutil_linux.c @@ -1022,6 +1022,69 @@ static void add_bond_info(struct netcf *ncf, }
+static void add_link_info(struct netcf *ncf, + const char *ifname, int ifindex ATTRIBUTE_UNUSED, + xmlDocPtr doc, xmlNodePtr root) { + char errbuf[128]; + xmlNodePtr link_node = NULL; + xmlAttrPtr prop = NULL; + char *path = NULL; + size_t length; + char *state = NULL; + char *speed = NULL; + char *nl; + + link_node = xml_node(doc, root, "link"); + ERR_NOMEM(link_node == NULL, ncf); + + xasprintf(&path, "/sys/class/net/%s/operstate", ifname); + ERR_NOMEM(!path, ncf); + state = read_file(path, &length); + FREE(path); + ERR_THROW_STRERROR(!state, ncf, EFILE, "Failed to read %s", path); + if ((nl = strchr(state, '\n'))) + *nl = 0; + prop = xmlSetProp(link_node, BAD_CAST "state", BAD_CAST state); + ERR_NOMEM(!prop, ncf); + + 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". + */ + 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); + } + + prop = xmlSetProp(link_node, BAD_CAST "speed", BAD_CAST speed); + ERR_NOMEM(prop == NULL, ncf); + + error: + FREE(path); + FREE(state); + FREE(speed); + return; +} + + static void add_type_specific_info(struct netcf *ncf, const char *ifname, int ifindex, xmlDocPtr doc, xmlNodePtr root) { @@ -1041,6 +1104,11 @@ static void add_type_specific_info(struct netcf *ncf, ERR_NOMEM(prop == NULL, ncf); }
+ if (iftype != NETCF_IFACE_TYPE_BRIDGE) { + add_link_info(ncf, ifname, ifindex, doc, root); + ERR_BAIL(ncf); + } + switch (iftype) { case NETCF_IFACE_TYPE_ETHERNET: add_ethernet_info(ncf, ifname, ifindex, doc, root); @@ -1061,6 +1129,7 @@ error: return; }
+ void add_state_to_xml_doc(struct netcf_if *nif, xmlDocPtr doc) { xmlNodePtr root; int ifindex, code;
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?
(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. But yeah, relying on the kernel numbers makes sense.
- 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?
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.
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!
netcf-devel@lists.fedorahosted.org