[libvirt] [PATCH v2 0/4] Expose link state & speed
While the 1/4 was ACKed, I'm sending int for completeness. And I've made a tiny change: the link speed is no longer unsigned long but unsigned int instead. That'll do too and it consumes less memory. Michal Privoznik (4): virInterface: Expose link state & speed virnetdev: Introduce virNetDevGetLinkInfo interface_backend_udev: Implement link speed & state node_device: Expose link state & speed docs/formatnode.html.in | 6 ++ docs/schemas/basictypes.rng | 25 ++++++ docs/schemas/interface.rng | 1 + docs/schemas/nodedev.rng | 1 + src/Makefile.am | 4 +- 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/conf/node_device_conf.c | 7 +- src/conf/node_device_conf.h | 2 + src/interface/interface_backend_udev.c | 5 ++ src/libvirt_private.syms | 5 ++ src/node_device/node_device_driver.c | 11 ++- src/node_device/node_device_udev.c | 5 ++ src/util/virnetdev.c | 104 ++++++++++++++++++++++++ src/util/virnetdev.h | 6 ++ tests/interfaceschemadata/bridge-no-address.xml | 1 + tests/interfaceschemadata/bridge.xml | 1 + tests/interfaceschemadata/ethernet-dhcp.xml | 1 + 20 files changed, 280 insertions(+), 7 deletions(-) -- 2.0.0
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@redhat.com> --- 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:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h Just so I understand - the reality is regardless of what the XML is on input - the code will still check/get/set the link state/speed - so this is mostly an output thing. It's not like if on input someone had "down" for the state that we'd attempt to set the link down... or if the speed on input was 500, but the file had 1000 - there's no changing of the file. John
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
Just so I understand - the reality is regardless of what the XML is on input - the code will still check/get/set the link state/speed - so this is mostly an output thing.
Yes. So far this is only for getting the link speed & state. Which brings up an interesting question. With recent issue with QoS on bridgeless networks on mind - should we make virInterface XML parsing actually reject any <link/> since it's output element only? One could argue that we usually ignore unknown elements, but then <link/> is not unknown element anymore. One way or another, it can be done in a separate patch.
It's not like if on input someone had "down" for the state that we'd attempt to set the link down... or if the speed on input was 500, but the file had 1000 - there's no changing of the file.
John
Michal
On Wed, Jun 11, 2014 at 11:19:14AM +0200, Michal Privoznik wrote:
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
Just so I understand - the reality is regardless of what the XML is on input - the code will still check/get/set the link state/speed - so this is mostly an output thing.
Yes. So far this is only for getting the link speed & state. Which brings up an interesting question. With recent issue with QoS on bridgeless networks on mind - should we make virInterface XML parsing actually reject any <link/> since it's output element only? One could argue that we usually ignore unknown elements, but then <link/> is not unknown element anymore. One way or another, it can be done in a separate patch.
Apps must always be able to round-trip XML. ie they should be able to do DumpXML and then feed that back into DefineXML and have no functional change. Thus we must not reject <link/> with an error when parsing, we must ignore it or honour it as appropriate for the desired semantics. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2014 12:28 PM, Daniel P. Berrange wrote:
On Wed, Jun 11, 2014 at 11:19:14AM +0200, Michal Privoznik wrote:
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
Just so I understand - the reality is regardless of what the XML is on input - the code will still check/get/set the link state/speed - so this is mostly an output thing. Yes. So far this is only for getting the link speed & state. Which brings up an interesting question. With recent issue with QoS on bridgeless networks on mind - should we make virInterface XML parsing actually reject any <link/> since it's output element only? One could argue that we usually ignore unknown elements, but then <link/> is not unknown element anymore. One way or another, it can be done in a separate patch. Apps must always be able to round-trip XML. ie they should be able to do DumpXML and then feed that back into DefineXML and have no functional change. Thus we must not reject <link/> with an error when parsing, we must ignore it or honour it as appropriate for the desired semantics.
I agree that <link> should just be ignored when defining a domain. But as a general response to that last paragraph, there are cases where doing a virDomainGetXMLDesc() on an active domain (without the INACTIVE flag), followed by a virDomainDefine using that output, will *not* produce the same domain. The example at the front of my mind is what happens with an <interface type='network'>, as related in my RFC email yesterday: https://www.redhat.com/archives/libvir-list/2014-June/msg00416.html but a more general problem is that if you run virDomainGetXMLDesc() with no INACTIVE flag, you will miss any previous edits to the domain definition since the last time it was started. This had actually been the cause of a long-running bug with "virsh net-edit" as a matter of fact. I had thought that what was required was the ability to roundtrip the *inactive* xml back into a define, not the status XML. If it's really a requirement that we be able to feed back the output of the status XML to get the exact same domain, then I think I've made a big mistake :-/
On Wed, Jun 11, 2014 at 01:04:02PM +0300, Laine Stump wrote:
On 06/11/2014 12:28 PM, Daniel P. Berrange wrote:
On Wed, Jun 11, 2014 at 11:19:14AM +0200, Michal Privoznik wrote:
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
Just so I understand - the reality is regardless of what the XML is on input - the code will still check/get/set the link state/speed - so this is mostly an output thing. Yes. So far this is only for getting the link speed & state. Which brings up an interesting question. With recent issue with QoS on bridgeless networks on mind - should we make virInterface XML parsing actually reject any <link/> since it's output element only? One could argue that we usually ignore unknown elements, but then <link/> is not unknown element anymore. One way or another, it can be done in a separate patch. Apps must always be able to round-trip XML. ie they should be able to do DumpXML and then feed that back into DefineXML and have no functional change. Thus we must not reject <link/> with an error when parsing, we must ignore it or honour it as appropriate for the desired semantics.
I agree that <link> should just be ignored when defining a domain. But as a general response to that last paragraph, there are cases where doing a virDomainGetXMLDesc() on an active domain (without the INACTIVE flag), followed by a virDomainDefine using that output, will *not* produce the same domain. The example at the front of my mind is what happens with an <interface type='network'>, as related in my RFC email yesterday:
https://www.redhat.com/archives/libvir-list/2014-June/msg00416.html
but a more general problem is that if you run virDomainGetXMLDesc() with no INACTIVE flag, you will miss any previous edits to the domain definition since the last time it was started. This had actually been the cause of a long-running bug with "virsh net-edit" as a matter of fact.
I had thought that what was required was the ability to roundtrip the *inactive* xml back into a define, not the status XML. If it's really a requirement that we be able to feed back the output of the status XML to get the exact same domain, then I think I've made a big mistake :-/
Yes, you're right actually. I should have clarified I meant inactive XML. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2014 01:11 PM, Daniel P. Berrange wrote:
On Wed, Jun 11, 2014 at 01:04:02PM +0300, Laine Stump wrote:
I had thought that what was required was the ability to roundtrip the *inactive* xml back into a define, not the status XML. If it's really a requirement that we be able to feed back the output of the status XML to get the exact same domain, then I think I've made a big mistake :-/ Yes, you're right actually. I should have clarified I meant inactive XML.
Whew! I feel much better now :-)
On 06/11/2014 05:19 AM, Michal Privoznik wrote:
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
So - I was just too lazy/tired to look last night, but formatdomain.html.in has an "Network interfaces" sub-section... Perhaps woefully under described though... John
On Wed, Jun 11, 2014 at 06:46:25AM -0400, John Ferlan wrote:
On 06/11/2014 05:19 AM, Michal Privoznik wrote:
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Already been ACK'd, but I will point out the interface.html.in hasn't been updated - later you update the nodedev.html.in file - so probably reasonable to do so again. While at it the consistency of using "Mbits" vs. "Mb" in the comment in device_conf.h
The first issue has simple explanation - there's no interface.html.in yet. Yes we lack the virInterface XML documentation. The second one I'm fixing prior to push.
So - I was just too lazy/tired to look last night, but formatdomain.html.in has an "Network interfaces" sub-section... Perhaps woefully under described though...
NB the network interfaces schema for virDomain (as documented in the formatdomain.html page) is different from the network interfaces schema for virInterface (as not documented :-) Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The purpose of this function is to fetch link state and link speed for given NIC name from the SYSFS. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 3 files changed, 111 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 394c086..6d08ee9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1540,6 +1540,7 @@ virNetDevClearIPv4Address; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetPhysicalFunction; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 3a60cf7..9d2c0d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1832,3 +1832,107 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, } #endif /* defined(__linux__) && defined(HAVE_LIBNL) */ + +#ifdef __linux__ +# define SYSFS_PREFIX "/sys/class/net/" +int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed) +{ + int ret = -1; + char *path = NULL; + char *buf = NULL; + char *tmp; + int tmp_state; + 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 + +int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed) +{ + /* Port me */ + VIR_DEBUG("Getting link info on %s is not implemented on this platform"); + if (state) + *state = 0; + if (speed) + *speed = 0; + return 0; +} +#endif /* defined(__linux__) */ diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 00c82e0..de33323 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -29,6 +29,7 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" +# include "device_conf.h" # ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -145,4 +146,9 @@ int virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, int *vf) ATTRIBUTE_NONNULL(1); +int virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_NETDEV_H__ */ -- 2.0.0
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
The purpose of this function is to fetch link state and link speed for given NIC name from the SYSFS.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 3 files changed, 111 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 394c086..6d08ee9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1540,6 +1540,7 @@ virNetDevClearIPv4Address; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetPhysicalFunction; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 3a60cf7..9d2c0d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1832,3 +1832,107 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, }
#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ + +#ifdef __linux__ +# define SYSFS_PREFIX "/sys/class/net/"
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.
+int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed)
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.
+{ + int ret = -1; + char *path = NULL; + char *buf = NULL; + char *tmp; + int tmp_state;
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 + +int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed)
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
+{ + /* Port me */ + VIR_DEBUG("Getting link info on %s is not implemented on this platform"); + if (state) + *state = 0; + if (speed) + *speed = 0; + return 0; +} +#endif /* defined(__linux__) */ diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 00c82e0..de33323 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -29,6 +29,7 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" +# include "device_conf.h"
# ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -145,4 +146,9 @@ int virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname, int *vf) ATTRIBUTE_NONNULL(1);
+int virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed) + ATTRIBUTE_NONNULL(1); + #endif /* __VIR_NETDEV_H__ */
On 11.06.2014 03:13, John Ferlan wrote:
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
The purpose of this function is to fetch link state and link speed for given NIC name from the SYSFS.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnetdev.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 3 files changed, 111 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 394c086..6d08ee9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1540,6 +1540,7 @@ virNetDevClearIPv4Address; virNetDevExists; virNetDevGetIndex; virNetDevGetIPv4Address; +virNetDevGetLinkInfo; virNetDevGetMAC; virNetDevGetMTU; virNetDevGetPhysicalFunction; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 3a60cf7..9d2c0d2 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1832,3 +1832,107 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED, }
#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ + +#ifdef __linux__ +# define SYSFS_PREFIX "/sys/class/net/"
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.
In fact, I've substituted both virAsprintf()s with virNetDevSysfsFile() which does exactly what I need.
+int +virNetDevGetLinkInfo(const char *ifname, + virInterfaceState *state, + unsigned int *speed)
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.
Right. The API design is a leftover from previous patch set where I was implementing netcf backend too. Fixed.
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.
I'd say error out in this function and let caller decide whether he wants to ignore error or not. If I make this function fault tolerant, caller loses that opportunity.
+{ + int ret = -1; + char *path = NULL; + char *buf = NULL; + char *tmp; + int tmp_state;
s/int/virInterfaceState/
In fact this is intentional. Remember Eric's TypeFromString() patches? The problem is, one can't be sure if compiler decides on using unsigned or signed integer to represent an enum. If it decides to use an unsigned int (IIRC that's the case of older gcc) then comparison a few lines below will never be true. That's why I'm introducing this dummy variable. I could as well assign the result to the destination variable directly. BTW: old compilers are the reason why I'm crippling some variable names, e.g. 'virInterfaceLinkPtr lnk' vs. 'virInterfaceLinkPtr link' as older gcc fails to see 'link' is a variable not the 'link()' function.
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.
Yeah, unit testing is certainly in place. But honestly, I thought it would blow the size of this patches up. Again, something that a follow up patch will do. Michal
On 11.06.2014 11:19, Michal Privoznik wrote:
<snip/> 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.
Yeah, unit testing is certainly in place. But honestly, I thought it would blow the size of this patches up. Again, something that a follow up patch will do.
So, now that I'm thinking about how to implement this I see two possible ways which I'm unable to choose between: 1) mock higher level functions like virNetDevSysfsFile() or virNetDevSetupControl(). So if a function wants to reach say "/sys/class/net/eth0/operstate" we can direct it into "tests/virnetdevdata/class/net/eth0/operstate" Pros: - small set of high level functions, so the code should be rather small. - we can keep whole sysfs dir struct in git Cons: - Unable to simulate some corner cases (e.g. EINVAL on read()). - keeping whole sysfs dir struct in git 2) mock lowlevel functions, like open(), ioctl(), etc. There's plenty of functions in virnetdev.c that use lowlevel functions like socket(), ioctl() to get data. Pros: - we can model kernel behavior more closely (e.g. even run time setting of some values like MAC/IP addressese, link state, you name it) - clean git Cons: - much more code I'm not hesitant to write the code, but I'd rather have an agreement on the design before that. Michal
On Wed, Jun 11, 2014 at 05:41:11PM +0200, Michal Privoznik wrote:
On 11.06.2014 11:19, Michal Privoznik wrote:
<snip/> 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.
Yeah, unit testing is certainly in place. But honestly, I thought it would blow the size of this patches up. Again, something that a follow up patch will do.
So, now that I'm thinking about how to implement this I see two possible ways which I'm unable to choose between:
1) mock higher level functions like virNetDevSysfsFile() or virNetDevSetupControl(). So if a function wants to reach say "/sys/class/net/eth0/operstate" we can direct it into "tests/virnetdevdata/class/net/eth0/operstate" Pros: - small set of high level functions, so the code should be rather small. - we can keep whole sysfs dir struct in git
Cons: - Unable to simulate some corner cases (e.g. EINVAL on read()). - keeping whole sysfs dir struct in git
2) mock lowlevel functions, like open(), ioctl(), etc. There's plenty of functions in virnetdev.c that use lowlevel functions like socket(), ioctl() to get data.
Pros: - we can model kernel behavior more closely (e.g. even run time setting of some values like MAC/IP addressese, link state, you name it) - clean git
Cons: - much more code
I'm not hesitant to write the code, but I'd rather have an agreement on the design before that.
To me the answer depends on what you're trying to test. If you're trying to test that functions in virnetdev.c do the right thing with syscalls/files they use, then mocking the lowlevel is best. If you're trying to test code that merely uses functions in virnetdev.c then it is valid to mock the virnetdev functions themselves. If you're trying to do both, then mocking the lowlevel functions will cope with both. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/11/2014 03:19 AM, Michal Privoznik wrote:
+{ + int ret = -1; + char *path = NULL; + char *buf = NULL; + char *tmp; + int tmp_state;
s/int/virInterfaceState/
In fact this is intentional. Remember Eric's TypeFromString() patches? The problem is, one can't be sure if compiler decides on using unsigned or signed integer to represent an enum. If it decides to use an unsigned int (IIRC that's the case of older gcc) then comparison a few lines below will never be true.
But at the least, you can do: int tmp_state /* virInterfaceState */ to make it easier for later readers to know valid values to store in the int. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/05/2014 06:39 PM, Michal Privoznik wrote:
+ + /* 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;
It appears the problem isn't quite so simple. On my system (Fedora 20, with a built-in realtek NIC, an Intel 82576-based SRIOV NIC, two bonds, a bridge device, a macvtap device, and several taps, when I look at /sys/class/net/$ifname/speed, I get the following results: (yes, I realized later on that the support you put into libvirt only reports link state for ethernets. Still it may make sense to report one or both items for some other types). * ANY device when ifconfiged down - attempts to read $ifname/speed return EINVAL. In addition, when *UP*: * lo/speed - EINVAL (note that lo self-identifies as type='ethernet' in netcf for some reason) * bridge - EINVAL (operstate seems to always be "down" for bridges, even if the bridge ifflags show UP|RUNNING.) * tap device - "10" * macvtap (physdev is Realtek) - physdev connected - "1000" - physdev disconnected - "10" (same thing reported by physdev) (about operstate - for a macvtap device, if the physdev is disconnected, the macvtap operstate is "lowerlayerdown", *not* "down") * bond - "4294967295" (if the physdevs (2 VFs) are disconnected) * bond - "1000" (2 physdevs connected) (interesting note - when I looked at the speed of bond0 (tying together two sriov VFs each with speed = 1000 using mode="802.3ad"), it showed "1000". At least one time when I ifconfiged both of the VFs down and back up, bond0/speed became "2000". I later could not reproduce this behavior.) (operstate: bonds with mode="activebackup" always show status as "down" and speed as "4294967295", but a bond with mode="802.3ad" properly reflects the physdev state) * RealTek ethernet - connected - "1000" - disconnected - "10" (operstate does show "down") * sriov-PF (Intel 82576) - up/disconnected - "65535" - up/connected - "1000" * sriov-VF (Intel 82576) - up/disconnected - "4294967295" - up/connected - "1000" So: 1) we need to allow for an EINVAL error, and report the speed as "0" in that case rather than logging an error. 2) just checking for ((unsigned int)-1) isn't good enough - notice that at least one driver (igb) returns 65535 when disconnected, and another returns "10" when disconnected. I think if the operstate is "down", then speed should unconditionally be 0. 3) we should probably report what we can for other interface types. In particular, the status for bond interfaces is useful in at least some modes of operation, and macvtap interfaces seem to reliably reflect the link state of their physdev. And maybe we should even detect bridges and report "unknown" or "indeterminate" or something as the state rather than "down". In my current working patch for netcf, I've accounted for 1, 2, and most of 3, don't know if it's better to leave the operstate of bridges alone or not. (likewise, at one point I was changing "lowerlayerdown" to "down", but then decided to leave that alone as well). Also, there is a larger problem with the XML that I just realized while making the netcf patch - you are only adding the <link> element to the toplevel document, so in the case of nested interfaces (e.g. two ethernets that are together in a bond, and the bond is attached to a bridge) you will only have it for the master (in the example, that would be the bridge, but of course you don't add that information for bridge or bond devices), but it really should be at all levels. For example: <interface name="br0" type="bridge"> <link state="down" speed="0"/> <bridge> <interface name="bond0" type="bond"> <link state="up" speed="2000"/> <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> (or, if you're against reporting anything for bridge devices, then remove that toplevel <link> element) I've made the netcf patch work this way, but libvirt strips out all but the toplevel. (note that in the case above, the interface xml for the ethernets is not available separate from the xml of the bridge - it is considered to be a single interface.)
On 12.06.2014 15:13, Laine Stump wrote:
On 06/05/2014 06:39 PM, Michal Privoznik wrote:
+ + /* 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;
It appears the problem isn't quite so simple.
On my system (Fedora 20, with a built-in realtek NIC, an Intel 82576-based SRIOV NIC, two bonds, a bridge device, a macvtap device, and several taps, when I look at /sys/class/net/$ifname/speed, I get the following results:
(yes, I realized later on that the support you put into libvirt only reports link state for ethernets. Still it may make sense to report one or both items for some other types).
* ANY device when ifconfiged down - attempts to read $ifname/speed return EINVAL.
In addition, when *UP*:
* lo/speed - EINVAL (note that lo self-identifies as type='ethernet' in netcf for some reason) * bridge - EINVAL
(operstate seems to always be "down" for bridges, even if the bridge ifflags show UP|RUNNING.)
* tap device - "10" * macvtap (physdev is Realtek) - physdev connected - "1000" - physdev disconnected - "10" (same thing reported by physdev)
(about operstate - for a macvtap device, if the physdev is disconnected, the macvtap operstate is "lowerlayerdown", *not* "down")
* bond - "4294967295" (if the physdevs (2 VFs) are disconnected) * bond - "1000" (2 physdevs connected)
(interesting note - when I looked at the speed of bond0 (tying together two sriov VFs each with speed = 1000 using mode="802.3ad"), it showed "1000". At least one time when I ifconfiged both of the VFs down and back up, bond0/speed became "2000". I later could not reproduce this behavior.)
(operstate: bonds with mode="activebackup" always show status as "down" and speed as "4294967295", but a bond with mode="802.3ad" properly reflects the physdev state)
* RealTek ethernet - connected - "1000" - disconnected - "10" (operstate does show "down")
* sriov-PF (Intel 82576) - up/disconnected - "65535" - up/connected - "1000"
* sriov-VF (Intel 82576) - up/disconnected - "4294967295" - up/connected - "1000"
Wow, so much analysis.
So:
1) we need to allow for an EINVAL error, and report the speed as "0" in that case rather than logging an error.
2) just checking for ((unsigned int)-1) isn't good enough - notice that at least one driver (igb) returns 65535 when disconnected, and another returns "10" when disconnected. I think if the operstate is "down", then speed should unconditionally be 0.
3) we should probably report what we can for other interface types. In particular, the status for bond interfaces is useful in at least some modes of operation, and macvtap interfaces seem to reliably reflect the link state of their physdev. And maybe we should even detect bridges and report "unknown" or "indeterminate" or something as the state rather than "down".
[I was going to start discussion here, but meanwhile it started on the patch I've sent "virNetDevGetLinkInfo: Don't report link speed if NIC's down" so I'll reply there] Michal
In the previous commit the helper function was prepared, so now we can wire it up and benefit from it. The Makefile change is required because we're including virnedev,h which includes virnetlink.h which tries to include netlink/msg.h. However this file is not under /usr/include directly but is dependent on libnl used. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/Makefile.am | 2 +- src/interface/interface_backend_udev.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index 01af164..a6b8d0b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1399,7 +1399,7 @@ endif ! WITH_DRIVER_MODULES libvirt_driver_interface_la_CFLAGS = \ -I$(top_srcdir)/src/access \ -I$(top_srcdir)/src/conf \ - $(AM_CFLAGS) + $(AM_CFLAGS) $(LIBNL_CFLAGS) libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_interface_la_LIBADD = if WITH_NETCF diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index c5353ea..f4de0e4 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -34,6 +34,7 @@ #include "viralloc.h" #include "virstring.h" #include "viraccessapicheck.h" +#include "virnetdev.h" #define VIR_FROM_THIS VIR_FROM_INTERFACE @@ -1059,6 +1060,10 @@ udevGetIfaceDef(struct udev *udev, const char *name) udev_device_get_sysattr_value(dev, "address")) < 0) goto error; + if (virNetDevGetLinkInfo(ifacedef->name, &ifacedef->lnk.state, + &ifacedef->lnk.speed) < 0) + goto error; + /* MTU */ mtu_str = udev_device_get_sysattr_value(dev, "mtu"); if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) { -- 2.0.0
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
In the previous commit the helper function was prepared, so now we can wire it up and benefit from it. The Makefile change is required because we're including virnedev,h which includes virnetlink.h which tries to include netlink/msg.h. However this file is not under /usr/include directly but is dependent on libnl used.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/Makefile.am | 2 +- src/interface/interface_backend_udev.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 01af164..a6b8d0b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1399,7 +1399,7 @@ endif ! WITH_DRIVER_MODULES libvirt_driver_interface_la_CFLAGS = \ -I$(top_srcdir)/src/access \ -I$(top_srcdir)/src/conf \ - $(AM_CFLAGS) + $(AM_CFLAGS) $(LIBNL_CFLAGS) libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_interface_la_LIBADD = if WITH_NETCF diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index c5353ea..f4de0e4 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -34,6 +34,7 @@ #include "viralloc.h" #include "virstring.h" #include "viraccessapicheck.h" +#include "virnetdev.h"
#define VIR_FROM_THIS VIR_FROM_INTERFACE
@@ -1059,6 +1060,10 @@ udevGetIfaceDef(struct udev *udev, const char *name) udev_device_get_sysattr_value(dev, "address")) < 0) goto error;
+ if (virNetDevGetLinkInfo(ifacedef->name, &ifacedef->lnk.state, + &ifacedef->lnk.speed) < 0) + goto error; +
NIT: Every other fetch has a comment :-) ACK John
/* MTU */ mtu_str = udev_device_get_sysattr_value(dev, "mtu"); if (virStrToLong_ui(mtu_str, NULL, 10, &mtu) < 0) {
While exposing the info under <interface/> in previous patch works, it may work only in cases where interface is configured on the host. However, orchestrating application may want to know the link state and speed even in that case. That's why we ought to expose this in nodedev XML too: virsh # nodedev-dumpxml net_eth0_f0_de_f1_2b_1b_f3 <device> <name>net_eth0_f0_de_f1_2b_1b_f3</name> <path>/sys/devices/pci0000:00/0000:00:19.0/net/eth0</path> <parent>pci_0000_00_19_0</parent> <capability type='net'> <interface>eth0</interface> <address>f0:de:f1:2b:1b:f3</address> <link speed='1000' state='up'/> <capability type='80203'/> </capability> </device> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatnode.html.in | 6 ++++++ docs/schemas/nodedev.rng | 1 + src/Makefile.am | 2 +- src/conf/node_device_conf.c | 7 ++++++- src/conf/node_device_conf.h | 2 ++ src/node_device/node_device_driver.c | 11 +++++++++-- src/node_device/node_device_udev.c | 5 +++++ 7 files changed, 30 insertions(+), 4 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index b424c96..0ef890f 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -154,6 +154,12 @@ <dd>The interface name tied to this device.</dd> <dt><code>address</code></dt> <dd>If present, the MAC address of the device.</dd> + <dt><code>link</code></dt> + <dd>Optional to reflect the status of the link. It has + two optional attributes: <code>speed</code> in Mbit per + second and <code>state</code> to tell the state of the + link. + </dd> <dt><code>capability</code></dt> <dd>A network protocol exposed by the device, where the attribute <code>type</code> can be "80203" for IEEE diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 81ab4d4..8c70536 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -233,6 +233,7 @@ <ref name='mac'/> </element> </optional> + <ref name="link-speed-state"/> <zeroOrMore> <ref name='subcapnet'/> diff --git a/src/Makefile.am b/src/Makefile.am index a6b8d0b..e7b3318 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1518,7 +1518,7 @@ libvirt_driver_nodedev_la_SOURCES = $(NODE_DEVICE_DRIVER_SOURCES) libvirt_driver_nodedev_la_CFLAGS = \ -I$(top_srcdir)/src/access \ -I$(top_srcdir)/src/conf \ - $(AM_CFLAGS) + $(AM_CFLAGS) $(LIBNL_CFLAGS) libvirt_driver_nodedev_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_nodedev_la_LIBADD = diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e65b5e4..8405a25 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -386,6 +386,7 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) if (data->net.address) virBufferEscapeString(&buf, "<address>%s</address>\n", data->net.address); + virInterfaceLinkFormat(&buf, &data->net.lnk); if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) { const char *subtyp = virNodeDevNetCapTypeToString(data->net.subtype); @@ -837,7 +838,7 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, xmlNodePtr node, union _virNodeDevCapData *data) { - xmlNodePtr orignode; + xmlNodePtr orignode, lnk; int ret = -1; char *tmp; @@ -869,6 +870,10 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, data->net.subtype = val; } + lnk = virXPathNode("./link", ctxt); + if (lnk && virInterfaceLinkParseXML(lnk, &data->net.lnk) < 0) + goto out; + ret = 0; out: ctxt->node = orignode; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 50e6805..462bfa4 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -29,6 +29,7 @@ # include "virutil.h" # include "virthread.h" # include "virpci.h" +# include "device_conf.h" # include <libxml/tree.h> @@ -135,6 +136,7 @@ struct _virNodeDevCapsDef { char *address; unsigned int address_len; char *ifname; + virInterfaceLink lnk; virNodeDevNetCapType subtype; /* LAST -> no subtype */ } net; struct { diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 6906463..92aeb45 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -39,6 +39,7 @@ #include "node_device_driver.h" #include "virutil.h" #include "viraccessapicheck.h" +#include "virnetdev.h" #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -47,10 +48,15 @@ static int update_caps(virNodeDeviceObjPtr dev) virNodeDevCapsDefPtr cap = dev->def->caps; while (cap) { - /* The only caps that currently need updating are FC related. */ if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST) { detect_scsi_host_caps(&dev->def->caps->data); } + if (cap->type == VIR_NODE_DEV_CAP_NET && + virNetDevGetLinkInfo(dev->def->caps->data.net.ifname, + &dev->def->caps->data.net.lnk.state, + &dev->def->caps->data.net.lnk.speed) < 0) + return -1; + cap = cap->next; } @@ -315,7 +321,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, goto cleanup; update_driver_name(obj); - update_caps(obj); + if (update_caps(obj) < 0) + goto cleanup; ret = virNodeDeviceDefFormat(obj->def); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9a951d9..0d94bba 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -40,6 +40,7 @@ #include "virfile.h" #include "virpci.h" #include "virstring.h" +#include "virnetdev.h" #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -667,6 +668,10 @@ static int udevProcessNetworkInterface(struct udev_device *device, goto out; } + if (virNetDevGetLinkInfo(data->net.ifname, &data->net.lnk.state, + &data->net.lnk.speed) < 0) + goto out; + ret = 0; out: -- 2.0.0
On 06/05/2014 11:39 AM, Michal Privoznik wrote:
While exposing the info under <interface/> in previous patch works, it may work only in cases where interface is configured on the host. However, orchestrating application may want to know the link state and speed even in that case. That's why we ought to expose this in nodedev XML too:
virsh # nodedev-dumpxml net_eth0_f0_de_f1_2b_1b_f3 <device> <name>net_eth0_f0_de_f1_2b_1b_f3</name> <path>/sys/devices/pci0000:00/0000:00:19.0/net/eth0</path> <parent>pci_0000_00_19_0</parent> <capability type='net'> <interface>eth0</interface> <address>f0:de:f1:2b:1b:f3</address> <link speed='1000' state='up'/> <capability type='80203'/> </capability> </device>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatnode.html.in | 6 ++++++ docs/schemas/nodedev.rng | 1 + src/Makefile.am | 2 +- src/conf/node_device_conf.c | 7 ++++++- src/conf/node_device_conf.h | 2 ++ src/node_device/node_device_driver.c | 11 +++++++++-- src/node_device/node_device_udev.c | 5 +++++ 7 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index b424c96..0ef890f 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -154,6 +154,12 @@ <dd>The interface name tied to this device.</dd> <dt><code>address</code></dt> <dd>If present, the MAC address of the device.</dd> + <dt><code>link</code></dt> + <dd>Optional to reflect the status of the link. It has + two optional attributes: <code>speed</code> in Mbit per
s/Mbit/Mbits
+ second and <code>state</code> to tell the state of the + link. + </dd>
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
<dt><code>capability</code></dt> <dd>A network protocol exposed by the device, where the attribute <code>type</code> can be "80203" for IEEE diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 81ab4d4..8c70536 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -233,6 +233,7 @@ <ref name='mac'/> </element> </optional> + <ref name="link-speed-state"/>
<zeroOrMore> <ref name='subcapnet'/> diff --git a/src/Makefile.am b/src/Makefile.am index a6b8d0b..e7b3318 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1518,7 +1518,7 @@ libvirt_driver_nodedev_la_SOURCES = $(NODE_DEVICE_DRIVER_SOURCES) libvirt_driver_nodedev_la_CFLAGS = \ -I$(top_srcdir)/src/access \ -I$(top_srcdir)/src/conf \ - $(AM_CFLAGS) + $(AM_CFLAGS) $(LIBNL_CFLAGS) libvirt_driver_nodedev_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_nodedev_la_LIBADD =
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e65b5e4..8405a25 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -386,6 +386,7 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) if (data->net.address) virBufferEscapeString(&buf, "<address>%s</address>\n", data->net.address); + virInterfaceLinkFormat(&buf, &data->net.lnk); if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) { const char *subtyp = virNodeDevNetCapTypeToString(data->net.subtype); @@ -837,7 +838,7 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, xmlNodePtr node, union _virNodeDevCapData *data) { - xmlNodePtr orignode; + xmlNodePtr orignode, lnk; int ret = -1; char *tmp;
@@ -869,6 +870,10 @@ virNodeDevCapNetParseXML(xmlXPathContextPtr ctxt, data->net.subtype = val; }
+ lnk = virXPathNode("./link", ctxt); + if (lnk && virInterfaceLinkParseXML(lnk, &data->net.lnk) < 0) + goto out; + ret = 0; out: ctxt->node = orignode; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 50e6805..462bfa4 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -29,6 +29,7 @@ # include "virutil.h" # include "virthread.h" # include "virpci.h" +# include "device_conf.h"
# include <libxml/tree.h>
@@ -135,6 +136,7 @@ struct _virNodeDevCapsDef { char *address; unsigned int address_len; char *ifname; + virInterfaceLink lnk; virNodeDevNetCapType subtype; /* LAST -> no subtype */ } net; struct { diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 6906463..92aeb45 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -39,6 +39,7 @@ #include "node_device_driver.h" #include "virutil.h" #include "viraccessapicheck.h" +#include "virnetdev.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -47,10 +48,15 @@ static int update_caps(virNodeDeviceObjPtr dev) virNodeDevCapsDefPtr cap = dev->def->caps;
while (cap) { - /* The only caps that currently need updating are FC related. */ if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST) { detect_scsi_host_caps(&dev->def->caps->data); } + if (cap->type == VIR_NODE_DEV_CAP_NET && + virNetDevGetLinkInfo(dev->def->caps->data.net.ifname, + &dev->def->caps->data.net.lnk.state, + &dev->def->caps->data.net.lnk.speed) < 0) + return -1; + cap = cap->next; }
@@ -315,7 +321,8 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr dev, goto cleanup;
update_driver_name(obj); - update_caps(obj); + if (update_caps(obj) < 0) + goto cleanup;
ret = virNodeDeviceDefFormat(obj->def);
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9a951d9..0d94bba 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -40,6 +40,7 @@ #include "virfile.h" #include "virpci.h" #include "virstring.h" +#include "virnetdev.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -667,6 +668,10 @@ static int udevProcessNetworkInterface(struct udev_device *device, goto out; }
+ if (virNetDevGetLinkInfo(data->net.ifname, &data->net.lnk.state, + &data->net.lnk.speed) < 0) + goto out; + ret = 0;
out:
On 05.06.2014 17:39, Michal Privoznik wrote:
While the 1/4 was ACKed, I'm sending int for completeness. And I've made a tiny change: the link speed is no longer unsigned long but unsigned int instead. That'll do too and it consumes less memory.
Michal Privoznik (4): virInterface: Expose link state & speed virnetdev: Introduce virNetDevGetLinkInfo interface_backend_udev: Implement link speed & state node_device: Expose link state & speed
docs/formatnode.html.in | 6 ++ docs/schemas/basictypes.rng | 25 ++++++ docs/schemas/interface.rng | 1 + docs/schemas/nodedev.rng | 1 + src/Makefile.am | 4 +- 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/conf/node_device_conf.c | 7 +- src/conf/node_device_conf.h | 2 + src/interface/interface_backend_udev.c | 5 ++ src/libvirt_private.syms | 5 ++ src/node_device/node_device_driver.c | 11 ++- src/node_device/node_device_udev.c | 5 ++ src/util/virnetdev.c | 104 ++++++++++++++++++++++++ src/util/virnetdev.h | 6 ++ tests/interfaceschemadata/bridge-no-address.xml | 1 + tests/interfaceschemadata/bridge.xml | 1 + tests/interfaceschemadata/ethernet-dhcp.xml | 1 + 20 files changed, 280 insertions(+), 7 deletions(-)
ping?
On 06/10/2014 08:19 PM, Michal Privoznik wrote:
On 05.06.2014 17:39, Michal Privoznik wrote:
While the 1/4 was ACKed, I'm sending int for completeness. And I've made a tiny change: the link speed is no longer unsigned long but unsigned int instead. That'll do too and it consumes less memory.
Michal Privoznik (4): virInterface: Expose link state & speed virnetdev: Introduce virNetDevGetLinkInfo interface_backend_udev: Implement link speed & state node_device: Expose link state & speed
FYI, I'm now working on adding this to netcf. I'll Cc you when I post the patch.
On 11.06.2014 10:02, Laine Stump wrote:
On 06/10/2014 08:19 PM, Michal Privoznik wrote:
On 05.06.2014 17:39, Michal Privoznik wrote:
While the 1/4 was ACKed, I'm sending int for completeness. And I've made a tiny change: the link speed is no longer unsigned long but unsigned int instead. That'll do too and it consumes less memory.
Michal Privoznik (4): virInterface: Expose link state & speed virnetdev: Introduce virNetDevGetLinkInfo interface_backend_udev: Implement link speed & state node_device: Expose link state & speed
FYI, I'm now working on adding this to netcf. I'll Cc you when I post the patch.
Awesome. Is the XML schema I'm proposing here okay with the netcf or do we need I need to change something? If so, it's still time to change it - up till the release. Michal
On 05.06.2014 17:39, Michal Privoznik wrote:
While the 1/4 was ACKed, I'm sending int for completeness. And I've made a tiny change: the link speed is no longer unsigned long but unsigned int instead. That'll do too and it consumes less memory.
Michal Privoznik (4): virInterface: Expose link state & speed virnetdev: Introduce virNetDevGetLinkInfo interface_backend_udev: Implement link speed & state node_device: Expose link state & speed
docs/formatnode.html.in | 6 ++ docs/schemas/basictypes.rng | 25 ++++++ docs/schemas/interface.rng | 1 + docs/schemas/nodedev.rng | 1 + src/Makefile.am | 4 +- 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/conf/node_device_conf.c | 7 +- src/conf/node_device_conf.h | 2 + src/interface/interface_backend_udev.c | 5 ++ src/libvirt_private.syms | 5 ++ src/node_device/node_device_driver.c | 11 ++- src/node_device/node_device_udev.c | 5 ++ src/util/virnetdev.c | 104 ++++++++++++++++++++++++ src/util/virnetdev.h | 6 ++ tests/interfaceschemadata/bridge-no-address.xml | 1 + tests/interfaceschemadata/bridge.xml | 1 + tests/interfaceschemadata/ethernet-dhcp.xml | 1 + 20 files changed, 280 insertions(+), 7 deletions(-)
So I've fixed all the nits John found and pushed. Thanks! Michal
participants (5)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Laine Stump -
Michal Privoznik