Currently it is not possible to determine the speed of an interface and whether a link is actually detected from the API. Orchestrating platforms want to be able to determine when the link has failed and where multiple speeds may be available which one the interface is actually connected at. This commit introduces an extension to our interface XML (without implementation to interface driver backends): <interface type='ethernet' name='eth0'> <start mode='none'/> <mac address='aa:bb:cc:dd:ee:ff'/> <link speed='1000' state='up'/> <mtu size='1492'/> ... </interface> Where @speed is negotiated link speed in Mbits per second, and state is the current NIC state (can be one of the following: "unknown", "notpresent", "down", "lowerlayerdown","testing", "dormant", "up"). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- docs/schemas/basictypes.rng | 25 ++++++++++ docs/schemas/interface.rng | 1 + src/conf/device_conf.c | 62 +++++++++++++++++++++++++ src/conf/device_conf.h | 27 ++++++++++- src/conf/interface_conf.c | 11 ++++- src/conf/interface_conf.h | 2 + src/libvirt_private.syms | 4 ++ tests/interfaceschemadata/bridge-no-address.xml | 1 + tests/interfaceschemadata/bridge.xml | 1 + tests/interfaceschemadata/ethernet-dhcp.xml | 1 + 10 files changed, 133 insertions(+), 2 deletions(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 34ef613..5fe3a97 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -397,4 +397,29 @@ </optional> </define> + <define name="link-speed-state"> + <optional> + <element name="link"> + <optional> + <attribute name="speed"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="state"> + <choice> + <value>unknown</value> + <value>notpresent</value> + <value>down</value> + <value>lowerlayerdown</value> + <value>testing</value> + <value>dormant</value> + <value>up</value> + </choice> + </attribute> + </optional> + </element> + </optional> + </define> + </grammar> diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index 3984b63..8e2218d 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -41,6 +41,7 @@ <attribute name="address"><ref name="macAddr"/></attribute> </element> </optional> + <ref name="link-speed-state"/> <!-- FIXME: Allow (some) ethtool options --> </define> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 317fdf2..6412d24 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -38,6 +38,13 @@ VIR_ENUM_IMPL(virDeviceAddressPCIMulti, "on", "off") +VIR_ENUM_IMPL(virInterfaceState, + VIR_INTERFACE_STATE_LAST, + "" /* value of zero means no state */, + "unknown", "notpresent", + "down", "lowerlayerdown", + "testing", "dormant", "up") + int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { /* PCI bus has 32 slots and 8 functions per slot */ @@ -142,3 +149,58 @@ virDevicePCIAddressEqual(virDevicePCIAddress *addr1, } return false; } + +int +virInterfaceLinkParseXML(xmlNodePtr node, + virInterfaceLinkPtr lnk) +{ + int ret = -1; + char *stateStr, *speedStr; + int state; + + stateStr = virXMLPropString(node, "state"); + speedStr = virXMLPropString(node, "speed"); + + if (stateStr) { + if ((state = virInterfaceStateTypeFromString(stateStr)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown link state: %s"), + stateStr); + goto cleanup; + } + lnk->state = state; + } + + if (speedStr && + virStrToLong_ui(speedStr, NULL, 10, &lnk->speed) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unable to parse link speed: %s"), + speedStr); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(stateStr); + VIR_FREE(speedStr); + return ret; +} + +int +virInterfaceLinkFormat(virBufferPtr buf, + const virInterfaceLink *lnk) +{ + if (!lnk->speed && !lnk->state) { + /* If there's nothing to format, return early. */ + return 0; + } + + virBufferAddLit(buf, "<link"); + if (lnk->speed) + virBufferAsprintf(buf, " speed='%u'", lnk->speed); + if (lnk->state) + virBufferAsprintf(buf, " state='%s'", + virInterfaceStateTypeToString(lnk->state)); + virBufferAddLit(buf, "/>\n"); + return 0; +} diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index d66afd2..be63ba7 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -40,6 +40,21 @@ typedef enum { VIR_DEVICE_ADDRESS_PCI_MULTI_LAST } virDeviceAddressPCIMulti; +VIR_ENUM_DECL(virDeviceAddressPCIMulti) + +typedef enum { + VIR_INTERFACE_STATE_UNKNOWN = 1, + VIR_INTERFACE_STATE_NOT_PRESENT, + VIR_INTERFACE_STATE_DOWN, + VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, + VIR_INTERFACE_STATE_TESTING, + VIR_INTERFACE_STATE_DORMANT, + VIR_INTERFACE_STATE_UP, + VIR_INTERFACE_STATE_LAST +} virInterfaceState; + +VIR_ENUM_DECL(virInterfaceState) + typedef struct _virDevicePCIAddress virDevicePCIAddress; typedef virDevicePCIAddress *virDevicePCIAddressPtr; struct _virDevicePCIAddress { @@ -50,6 +65,13 @@ struct _virDevicePCIAddress { int multi; /* enum virDomainDeviceAddressPCIMulti */ }; +typedef struct _virInterfaceLink virInterfaceLink; +typedef virInterfaceLink *virInterfaceLinkPtr; +struct _virInterfaceLink { + virInterfaceState state; /* link state */ + unsigned int speed; /* link speed in Mb per second */ +}; + int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); int virDevicePCIAddressParseXML(xmlNodePtr node, @@ -62,7 +84,10 @@ int virDevicePCIAddressFormat(virBufferPtr buf, bool virDevicePCIAddressEqual(virDevicePCIAddress *addr1, virDevicePCIAddress *addr2); +int virInterfaceLinkParseXML(xmlNodePtr node, + virInterfaceLinkPtr lnk); -VIR_ENUM_DECL(virDeviceAddressPCIMulti) +int virInterfaceLinkFormat(virBufferPtr buf, + const virInterfaceLink *lnk); #endif /* __DEVICE_CONF_H__ */ diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 1f67446..2b3f699 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -705,12 +705,19 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, int parentIfType) } def->type = type; switch (type) { - case VIR_INTERFACE_TYPE_ETHERNET: + case VIR_INTERFACE_TYPE_ETHERNET: { + xmlNodePtr lnk; + if (virInterfaceDefParseName(def, ctxt) < 0) goto error; tmp = virXPathString("string(./mac/@address)", ctxt); if (tmp != NULL) def->mac = tmp; + + lnk = virXPathNode("./link", ctxt); + if (lnk && virInterfaceLinkParseXML(lnk, &def->lnk) < 0) + goto error; + if (parentIfType == VIR_INTERFACE_TYPE_LAST) { /* only recognize these in toplevel bond interfaces */ if (virInterfaceDefParseStartMode(def, ctxt) < 0) @@ -721,6 +728,7 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, int parentIfType) goto error; } break; + } case VIR_INTERFACE_TYPE_BRIDGE: { xmlNodePtr bridge; @@ -1088,6 +1096,7 @@ virInterfaceDefDevFormat(virBufferPtr buf, const virInterfaceDef *def) virInterfaceStartmodeDefFormat(buf, def->startmode); if (def->mac != NULL) virBufferAsprintf(buf, "<mac address='%s'/>\n", def->mac); + virInterfaceLinkFormat(buf, &def->lnk); if (def->mtu != 0) virBufferAsprintf(buf, "<mtu size='%d'/>\n", def->mtu); virInterfaceProtocolDefFormat(buf, def); diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index b3c92b2..94c18ef 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -31,6 +31,7 @@ # include "internal.h" # include "virutil.h" # include "virthread.h" +# include "device_conf.h" /* There is currently 3 types of interfaces */ @@ -146,6 +147,7 @@ struct _virInterfaceDef { char *name; /* interface name */ unsigned int mtu; /* maximum transmit size in byte */ char *mac; /* MAC address */ + virInterfaceLink lnk; /* interface link info */ virInterfaceStartMode startmode; /* how it is started */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d73a9f5..394c086 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -83,6 +83,10 @@ virDevicePCIAddressEqual; virDevicePCIAddressFormat; virDevicePCIAddressIsValid; virDevicePCIAddressParseXML; +virInterfaceLinkFormat; +virInterfaceLinkParseXML; +virInterfaceStateTypeFromString; +virInterfaceStateTypeToString; # conf/domain_addr.h diff --git a/tests/interfaceschemadata/bridge-no-address.xml b/tests/interfaceschemadata/bridge-no-address.xml index 7757534..68b8c94 100644 --- a/tests/interfaceschemadata/bridge-no-address.xml +++ b/tests/interfaceschemadata/bridge-no-address.xml @@ -4,6 +4,7 @@ <bridge stp='off'> <interface type='ethernet' name='eth0'> <mac address='ab:bb:cc:dd:ee:ff'/> + <link speed='1000' state='up'/> </interface> <interface type='ethernet' name='eth1'> </interface> diff --git a/tests/interfaceschemadata/bridge.xml b/tests/interfaceschemadata/bridge.xml index 2535edf..c865116 100644 --- a/tests/interfaceschemadata/bridge.xml +++ b/tests/interfaceschemadata/bridge.xml @@ -7,6 +7,7 @@ <bridge stp='off' delay='0.01'> <interface type='ethernet' name='eth0'> <mac address='ab:bb:cc:dd:ee:ff'/> + <link speed='10'/> </interface> <interface type='ethernet' name='eth1'> </interface> diff --git a/tests/interfaceschemadata/ethernet-dhcp.xml b/tests/interfaceschemadata/ethernet-dhcp.xml index fe969df..c124372 100644 --- a/tests/interfaceschemadata/ethernet-dhcp.xml +++ b/tests/interfaceschemadata/ethernet-dhcp.xml @@ -1,6 +1,7 @@ <interface type='ethernet' name='eth0'> <start mode='none'/> <mac address='aa:bb:cc:dd:ee:ff'/> + <link state='down'/> <mtu size='1492'/> <protocol family='ipv4'> <dhcp peerdns='no'/> -- 2.0.0

On 06/05/2014 11:39 AM, Michal Privoznik wrote: There is a 'NET_SYSFS' definition earlier to the same path - seems it could/should be reused... NIT: Your definition will create a double slash below for operstate and speed... Whether that's desired or not is up to you... There's a mix of usage within libvirt sources. Could there ever be any other stats/data to fetch? Perhaps consider passing a virInterfaceLinkPtr instead and then reference state/speed off of it. Then in the future you don't have to keep adding params to the API. None of the current callers pass NULL for either parameter. I also wonder if we really want to fail out if we something goes wrong. It's not like we're affecting the state/speed of the device if something did go wrong - we're just outputting data if it's there. I guess I'm thinking more about the callers, but that's a design decision you have to make and live with. Considering the comment below regarding the bug with speed - one wonders what else lurks in former formats that the following code can error out on. s/int/virInterfaceState/ Seems we should be able to set up a dummy /sys/class/net environment in order to test the new API's... Something I attempted with my recently posted scsi_host changes. > + unsigned int tmp_speed; > + > + if (state) { > + if (virAsprintf(&path, SYSFS_PREFIX "%s/operstate", ifname) < 0) > + goto cleanup; > + > + if (virFileReadAll(path, 1024, &buf) < 0) { > + virReportSystemError(errno, > + _("unable to read: %s"), > + path); > + goto cleanup; > + } > + > + if (!(tmp = strchr(buf, '\n'))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse: %s"), > + buf); > + goto cleanup; > + } > + > + *tmp = '\0'; > + > + /* We shouldn't allow 0 here, because > + * virInterfaceState enum starts from 1. */ > + if ((tmp_state = virInterfaceStateTypeFromString(buf)) <= 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse: %s"), > + buf); > + goto cleanup; > + } > + > + *state = tmp_state; > + > + VIR_FREE(path); > + VIR_FREE(buf); > + } > + > + if (speed) { > + if (virAsprintf(&path, SYSFS_PREFIX "%s/speed", ifname) < 0) > + goto cleanup; > + > + if (virFileReadAll(path, 1024, &buf) < 0) { > + /* Some devices doesn't report speed, in which case we get EINVAL */ > + if (errno == EINVAL) { > + ret = 0; > + goto cleanup; > + } > + virReportSystemError(errno, > + _("unable to read: %s"), > + path); > + goto cleanup; > + } > + > + if (virStrToLong_ui(buf, &tmp, 10, &tmp_speed) < 0 || > + *tmp != '\n') { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse: %s"), > + buf); > + goto cleanup; > + } > + > + /* 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. */ > + *speed = (int) tmp_speed == -1 ? 0 : tmp_speed; > + } > + > + ret = 0; > + cleanup: > + VIR_FREE(buf); > + VIR_FREE(path); > + return ret; > +} > + > +#else > + In virutil.c each of the paremters has a "ATTRIBUTE_UNUSED" and the code is: virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return -1; Your call on how do do this ACK - in general though based on fixing up the nits/minor issues. I trust if you change the parameters that it'll be done right or the compiler will complain! John

On 06/05/2014 11:39 AM, Michal Privoznik wrote: NIT: Every other fetch has a comment :-) ACK John

On 06/05/2014 11:39 AM, Michal Privoznik wrote: s/Mbit/Mbits Should it be noted that reality is this is an output XML setting? Although possibly set at read time, it's essentially ignored. The rest seems reasonable. ACK John