[libvirt] [PATCH v3 0/4] Gathering network interface statistics with openvswitch
This new version removes the virstat.c from locale configuration. It also adds a new commit to autodected the ifname of the ovs vhostuser. Mehdi Abaakouk (4): Gathering vhostuser interface stats with ovs virstat: fix signature of virstat helper Move virstat.c code to virnetdevtap.c domain_conf: autodetect vhostuser ifname po/POTFILES.in | 1 - src/Makefile.am | 1 - src/conf/domain_conf.c | 10 +++ src/libvirt_private.syms | 5 +- src/libxl/libxl_driver.c | 4 +- src/lxc/lxc_driver.c | 3 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_driver.c | 31 +++++-- src/uml/uml_driver.c | 1 - src/util/virnetdevopenvswitch.c | 106 ++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 + src/util/virnetdevtap.c | 143 ++++++++++++++++++++++++++++++++ src/util/virnetdevtap.h | 3 + src/util/virstats.c | 178 ---------------------------------------- src/util/virstats.h | 31 ------- src/xen/xen_hypervisor.c | 4 +- 16 files changed, 298 insertions(+), 231 deletions(-) delete mode 100644 src/util/virstats.c delete mode 100644 src/util/virstats.h -- 2.10.2
From: Mehdi Abaakouk <sileht@redhat.com> When vhostuser interfaces are used, the interface statistics are not available in /proc/net/dev. This change looks at the openvswitch interfaces statistics tables to provide this information for vhostuser interface. Note that in openvswitch world drop/error doesn't always make sense for some interface type. When these informations are not available we set them to 0 on the virDomainInterfaceStats. --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 29 ++++++++--- src/util/virnetdevopenvswitch.c | 106 ++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 ++ 4 files changed, 133 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index baff82b..aa27f78 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2034,6 +2034,7 @@ virNetDevMidonetUnbindPort; # util/virnetdevopenvswitch.h virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; +virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87ca09d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -66,6 +66,7 @@ #include "virhostcpu.h" #include "virhostmem.h" #include "virstats.h" +#include "virnetdevopenvswitch.h" #include "capabilities.h" #include "viralloc.h" #include "viruuid.h" @@ -10975,6 +10976,7 @@ qemuDomainInterfaceStats(virDomainPtr dom, virDomainInterfaceStatsPtr stats) { virDomainObjPtr vm; + virDomainNetDefPtr net = NULL; size_t i; int ret = -1; @@ -10994,16 +10996,21 @@ qemuDomainInterfaceStats(virDomainPtr dom, for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->ifname && STREQ(vm->def->nets[i]->ifname, path)) { - ret = 0; + net = vm->def->nets[i]; break; } } - if (ret == 0) - ret = virNetInterfaceStats(path, stats); - else + if (net) { + if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + ret = virNetDevOpenvswitchInterfaceStats(path, stats); + } else { + ret = virNetInterfaceStats(path, stats); + } + } else { virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); + } cleanup: virDomainObjEndAPI(&vm); @@ -19140,9 +19147,17 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, QEMU_ADD_NAME_PARAM(record, maxparams, "net", "name", i, dom->def->nets[i]->ifname); - if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { - virResetLastError(); - continue; + if (dom->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + if (virNetDevOpenvswitchInterfaceStats(dom->def->nets[i]->ifname, + &tmp) < 0) { + virResetLastError(); + continue; + } + } else { + if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + virResetLastError(); + continue; + } } QEMU_ADD_NET_PARAM(record, maxparams, i, diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 9283bbb..db8b542 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -24,6 +24,8 @@ #include <config.h> +#include <stdio.h> + #include "virnetdevopenvswitch.h" #include "vircommand.h" #include "viralloc.h" @@ -270,3 +272,107 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchInterfaceStats: + * @ifname: the name of the interface + * @stats: the retreived domain interface stat + * + * Retrieves the OVS interfaces stats + * + * Returns 0 in case of success or -1 in case of failure + */ +int +virNetDevOpenvswitchInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) +{ + virCommandPtr cmd = NULL; + char *output; + long long rx_bytes; + long long rx_packets; + long long tx_bytes; + long long tx_packets; + long long rx_errs; + long long rx_drop; + long long tx_errs; + long long tx_drop; + int ret = -1; + + // Just ensure the interface exists in ovs + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "name", NULL); + virCommandSetOutputBuffer(cmd, &output); + + if (virCommandRun(cmd, NULL) < 0) { + // no ovs-vsctl or interface 'ifname' doesn't exists in ovs + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface not found")); + goto cleanup; + } + + VIR_FREE(output); + virCommandFree(cmd); + + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "statistics:rx_bytes", + "statistics:rx_packets", + "statistics:tx_bytes", + "statistics:tx_packets", NULL); + virCommandSetOutputBuffer(cmd, &output); + + if (virCommandRun(cmd, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface doesn't have statistics")); + goto cleanup; + } + + // The TX/RX fields appear to be swapped here because this is the host view. + if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n", + &tx_bytes, &tx_packets, &rx_bytes, &rx_packets) != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fail to parse ovs-vsctl output")); + goto cleanup; + } + + stats->rx_bytes = rx_bytes; + stats->rx_packets = rx_packets; + stats->tx_bytes = tx_bytes; + stats->tx_packets = tx_packets; + + VIR_FREE(output); + virCommandFree(cmd); + + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "statistics:rx_errors", + "statistics:rx_dropped", + "statistics:tx_errors", + "statistics:tx_dropped", NULL); + virCommandSetOutputBuffer(cmd, &output); + if (virCommandRun(cmd, NULL) < 0) { + // This interface don't have errors or dropped, so set them to 0 + stats->rx_errs = 0; + stats->rx_drop = 0; + stats->tx_errs = 0; + stats->tx_drop = 0; + } else if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n", + &tx_errs, &tx_drop, &rx_errs, &rx_drop) == 4) { + stats->rx_errs = rx_errs; + stats->rx_drop = rx_drop; + stats->tx_errs = tx_errs; + stats->tx_drop = tx_drop; + ret = 0; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fail to parse ovs-vsctl output")); + goto cleanup; + } + ret = 0; + + cleanup: + VIR_FREE(output); + virCommandFree(cmd); + return ret; +} diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 131be73..0f9e1df 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -48,4 +48,8 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevOpenvswitchInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */ -- 2.10.2
On 18.11.2016 23:51, Mehdi Abaakouk wrote:
From: Mehdi Abaakouk <sileht@redhat.com>
When vhostuser interfaces are used, the interface statistics are not available in /proc/net/dev.
This change looks at the openvswitch interfaces statistics tables to provide this information for vhostuser interface.
Note that in openvswitch world drop/error doesn't always make sense for some interface type. When these informations are not available we set them to 0 on the virDomainInterfaceStats. --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 29 ++++++++--- src/util/virnetdevopenvswitch.c | 106 ++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 ++ 4 files changed, 133 insertions(+), 7 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index baff82b..aa27f78 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2034,6 +2034,7 @@ virNetDevMidonetUnbindPort; # util/virnetdevopenvswitch.h virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; +virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87ca09d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -66,6 +66,7 @@ #include "virhostcpu.h" #include "virhostmem.h" #include "virstats.h" +#include "virnetdevopenvswitch.h" #include "capabilities.h" #include "viralloc.h" #include "viruuid.h" @@ -10975,6 +10976,7 @@ qemuDomainInterfaceStats(virDomainPtr dom, virDomainInterfaceStatsPtr stats) { virDomainObjPtr vm; + virDomainNetDefPtr net = NULL; size_t i; int ret = -1;
@@ -10994,16 +10996,21 @@ qemuDomainInterfaceStats(virDomainPtr dom, for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->ifname && STREQ(vm->def->nets[i]->ifname, path)) { - ret = 0; + net = vm->def->nets[i]; break; } }
- if (ret == 0) - ret = virNetInterfaceStats(path, stats); - else + if (net) { + if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + ret = virNetDevOpenvswitchInterfaceStats(path, stats); + } else { + ret = virNetInterfaceStats(path, stats); + } + } else { virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); + }
Oh my. Not your fault but this looks ugly. It has even before you've touched it. BTW: maybe I am misreading something but my understanding of vhost-user is that it can be plugged into any type of bridge (e.g. snabb). How does this work then if we run ovs-vsctl then? Do you perhaps have a set of steps how can I test this feature? Because so far I've used vhost-user-bridge helper from qemu repo but this will not work with it.
cleanup: virDomainObjEndAPI(&vm); @@ -19140,9 +19147,17 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, QEMU_ADD_NAME_PARAM(record, maxparams, "net", "name", i, dom->def->nets[i]->ifname);
- if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { - virResetLastError(); - continue; + if (dom->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + if (virNetDevOpenvswitchInterfaceStats(dom->def->nets[i]->ifname, + &tmp) < 0) { + virResetLastError(); + continue; + } + } else { + if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + virResetLastError(); + continue; + } }
QEMU_ADD_NET_PARAM(record, maxparams, i, diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 9283bbb..db8b542 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -24,6 +24,8 @@
#include <config.h>
+#include <stdio.h> + #include "virnetdevopenvswitch.h" #include "vircommand.h" #include "viralloc.h" @@ -270,3 +272,107 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) virCommandFree(cmd); return ret; } + +/** + * virNetDevOpenvswitchInterfaceStats: + * @ifname: the name of the interface + * @stats: the retreived domain interface stat + * + * Retrieves the OVS interfaces stats + * + * Returns 0 in case of success or -1 in case of failure + */ +int +virNetDevOpenvswitchInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) +{ + virCommandPtr cmd = NULL; + char *output; + long long rx_bytes; + long long rx_packets; + long long tx_bytes; + long long tx_packets; + long long rx_errs; + long long rx_drop; + long long tx_errs; + long long tx_drop; + int ret = -1; + + // Just ensure the interface exists in ovs + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "name", NULL); + virCommandSetOutputBuffer(cmd, &output); + + if (virCommandRun(cmd, NULL) < 0) { + // no ovs-vsctl or interface 'ifname' doesn't exists in ovs
We prefer c89 style of comments.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface not found")); + goto cleanup; + } + + VIR_FREE(output); + virCommandFree(cmd); + + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "statistics:rx_bytes", + "statistics:rx_packets", + "statistics:tx_bytes", + "statistics:tx_packets", NULL); + virCommandSetOutputBuffer(cmd, &output); + + if (virCommandRun(cmd, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface doesn't have statistics")); + goto cleanup; + } + + // The TX/RX fields appear to be swapped here because this is the host view. + if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n", + &tx_bytes, &tx_packets, &rx_bytes, &rx_packets) != 4) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fail to parse ovs-vsctl output")); + goto cleanup; + } + + stats->rx_bytes = rx_bytes; + stats->rx_packets = rx_packets; + stats->tx_bytes = tx_bytes; + stats->tx_packets = tx_packets; + + VIR_FREE(output); + virCommandFree(cmd); + + cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", + "get", "Interface", ifname, + "statistics:rx_errors", + "statistics:rx_dropped", + "statistics:tx_errors", + "statistics:tx_dropped", NULL); + virCommandSetOutputBuffer(cmd, &output); + if (virCommandRun(cmd, NULL) < 0) { + // This interface don't have errors or dropped, so set them to 0 + stats->rx_errs = 0; + stats->rx_drop = 0; + stats->tx_errs = 0; + stats->tx_drop = 0; + } else if (sscanf(output, "%lld\n%lld\n%lld\n%lld\n", + &tx_errs, &tx_drop, &rx_errs, &rx_drop) == 4) { + stats->rx_errs = rx_errs; + stats->rx_drop = rx_drop; + stats->tx_errs = tx_errs; + stats->tx_drop = tx_drop; + ret = 0; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fail to parse ovs-vsctl output")); + goto cleanup; + } + ret = 0; + + cleanup: + VIR_FREE(output); + virCommandFree(cmd); + return ret; +}
Michal
- if (ret == 0) - ret = virNetInterfaceStats(path, stats); - else + if (net) { + if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + ret = virNetDevOpenvswitchInterfaceStats(path, stats); + } else { + ret = virNetInterfaceStats(path, stats); + } + } else { virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); + }
Oh my. Not your fault but this looks ugly. It has even before you've touched it.
BTW: maybe I am misreading something but my understanding of vhost-user is that it can be plugged into any type of bridge (e.g. snabb). How does this work then if we run ovs-vsctl then?
I don't think you misreading, vhostuser can be created by anything it's just a unix-socket in this end. And libvirt only known the location of this socket and not how it have been created. libvirt was already guessing at getting the statistics, by 'trying' with /proc/net/dev even that doesn't make any sense for vhostuser interface. Now I just 'try' with ovs-vsctl but perhaps that doesn't make sense too if the unix-socket have not been created by openvswitch. Since libvirt have no real control of how a (host) network interface is created. It can only guess the statistics location. My change just adds a new way/tool to guess that.
Do you perhaps have a set of steps how can I test this feature? Because so far I've used vhost-user-bridge helper from qemu repo but this will not work with it.
About testing, my setup looks like: I install the dpdk variant of openvswitch and dpdk tools I enable dpdk support in ovs with: (This can be a bit different depending on OS and openvswitch version) $ ovs-vsctl set Open_vSwitch . "other_config:dpdk-init=true" $ ovs-vsctl set Open_vSwitch . "other_config:dpdk-alloc-mem=2048" $ ovs-vsctl set Open_vSwitch . "other_config:dpdk-extra=--vhost-owner libvirt-qemu:kvm --vhost-perm 0666" $ systemctl restart openvswitch-switch I unbind a network card (virtio network card work well with recent dpdk) on my host with command like: $ dpdk-devbind -u 0000:00:04.0 (previously called dpdk_nic_bind, too) I create a bridge with two interfaces. dpdk0 is the first unbind interface. dpdkvhostuser0 is a vhostuser unix-socket located /var/run/openvswitch/dpdkvhostuser0 $ ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk $ ovs-vsctl add-port br0 dpdkvhostuser0 -- set Interface dpdkvhostuser0 type=dpdkvhostuser And I allow packets to pass from/to each interfaces $ ovs-ofctl del-flows br0 $ ovs-ofctl add-flow br0 in_port=1,action=output:2 $ ovs-ofctl add-flow br0 in_port=2,action=output:1 Then I create a VM with a network interface that looks like: <interface type='vhostuser'> <target dev='dpdkvhostuser0'/> <mac address='52:54:00:3b:83:1b'/> <source type='unix' path='/var/run/openvswitch/dpdkvhostuser0' mode='client'/> <model type='virtio'/> <driver queues='2'> <host mrg_rxbuf='off'/> </driver> </interface> I do some ping command to make statistics filled Test with: $ ovs-vsctl get Interface dpdkvhostuser0 statistics $ virsh domifstat <vmname> dpdkvhostuser0 Cheers, -- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
On 08.12.2016 15:35, Mehdi Abaakouk wrote:
- if (ret == 0) - ret = virNetInterfaceStats(path, stats); - else + if (net) { + if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { + ret = virNetDevOpenvswitchInterfaceStats(path, stats); + } else { + ret = virNetInterfaceStats(path, stats); + } + } else { virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); + }
Oh my. Not your fault but this looks ugly. It has even before you've touched it.
BTW: maybe I am misreading something but my understanding of vhost-user is that it can be plugged into any type of bridge (e.g. snabb). How does this work then if we run ovs-vsctl then?
I don't think you misreading, vhostuser can be created by anything it's just a unix-socket in this end. And libvirt only known the location of this socket and not how it have been created.
libvirt was already guessing at getting the statistics, by 'trying' with /proc/net/dev even that doesn't make any sense for vhostuser interface. Now I just 'try' with ovs-vsctl but perhaps that doesn't make sense too if the unix-socket have not been created by openvswitch.
Since libvirt have no real control of how a (host) network interface is created. It can only guess the statistics location. My change just adds a new way/tool to guess that.
Fair enough.
Do you perhaps have a set of steps how can I test this feature? Because so far I've used vhost-user-bridge helper from qemu repo but this will not work with it.
About testing, my setup looks like:
I install the dpdk variant of openvswitch and dpdk tools
I enable dpdk support in ovs with: (This can be a bit different depending on OS and openvswitch version)
$ ovs-vsctl set Open_vSwitch . "other_config:dpdk-init=true" $ ovs-vsctl set Open_vSwitch . "other_config:dpdk-alloc-mem=2048" $ ovs-vsctl set Open_vSwitch . "other_config:dpdk-extra=--vhost-owner libvirt-qemu:kvm --vhost-perm 0666" $ systemctl restart openvswitch-switch
I unbind a network card (virtio network card work well with recent dpdk) on my host with command like: $ dpdk-devbind -u 0000:00:04.0 (previously called dpdk_nic_bind, too)
I create a bridge with two interfaces. dpdk0 is the first unbind interface. dpdkvhostuser0 is a vhostuser unix-socket located /var/run/openvswitch/dpdkvhostuser0
$ ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev $ ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk $ ovs-vsctl add-port br0 dpdkvhostuser0 -- set Interface dpdkvhostuser0 type=dpdkvhostuser
And I allow packets to pass from/to each interfaces
$ ovs-ofctl del-flows br0 $ ovs-ofctl add-flow br0 in_port=1,action=output:2 $ ovs-ofctl add-flow br0 in_port=2,action=output:1
Then I create a VM with a network interface that looks like:
<interface type='vhostuser'> <target dev='dpdkvhostuser0'/> <mac address='52:54:00:3b:83:1b'/> <source type='unix' path='/var/run/openvswitch/dpdkvhostuser0' mode='client'/> <model type='virtio'/> <driver queues='2'> <host mrg_rxbuf='off'/> </driver> </interface>
I do some ping command to make statistics filled
Test with:
$ ovs-vsctl get Interface dpdkvhostuser0 statistics $ virsh domifstat <vmname> dpdkvhostuser0
Cheers,
Awesome. This is working for me. Therefore I went ahead, fixed all the small nits raised during review and pushed patches 1-3. As discussed in patch 4, we probably want different approach anyway so I have not pushed that one. Congratulations on your first libvirt contribution! Michal
On Fri, Dec 09, 2016 at 10:36:32AM +0100, Michal Privoznik wrote:
Awesome. This is working for me.
Therefore I went ahead, fixed all the small nits raised during review and pushed patches 1-3. As discussed in patch 4, we probably want different approach anyway so I have not pushed that one.
Congratulations on your first libvirt contribution!
Thank you for the quick processing of the changes! -- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
From: Mehdi Abaakouk <sileht@redhat.com> In preparation to the code move to virnetdevtap.c, this change: * renames virNetInterfaceStats to virNetDevTapInterfaceStats * changes 'path' to 'ifname', to use the same vocable as other method in virnetdevtap.c. * Add the attributes checker --- src/libvirt_private.syms | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/util/virstats.c | 22 +++++++++++----------- src/util/virstats.h | 5 +++-- src/xen/xen_hypervisor.c | 2 +- 8 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa27f78..0036cbd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2367,7 +2367,7 @@ virSocketAddrSetIPv6AddrNetOrder; virSocketAddrSetPort; # util/virstats.h -virNetInterfaceStats; +virNetDevTapInterfaceStats; # util/virstorageencryption.h virStorageEncryptionFormat; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b2f3b16..67f0e58 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4982,7 +4982,7 @@ libxlDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("'%s' is not a known interface"), path); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4a0165a..526d40d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2893,7 +2893,7 @@ lxcDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("Invalid path, '%s' is not a known interface"), path); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 38a562e..7bd3acf 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2024,7 +2024,7 @@ openvzDomainInterfaceStats(virDomainPtr dom, } if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87ca09d..38208b1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11005,7 +11005,7 @@ qemuDomainInterfaceStats(virDomainPtr dom, if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { ret = virNetDevOpenvswitchInterfaceStats(path, stats); } else { - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); } } else { virReportError(VIR_ERR_INVALID_ARG, @@ -19154,7 +19154,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, continue; } } else { - if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + if (virNetDevTapInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { virResetLastError(); continue; } diff --git a/src/util/virstats.c b/src/util/virstats.c index c4725ed..95b4c38 100644 --- a/src/util/virstats.c +++ b/src/util/virstats.c @@ -50,10 +50,10 @@ */ #ifdef __linux__ int -virNetInterfaceStats(const char *path, - virDomainInterfaceStatsPtr stats) +virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) { - int path_len; + int ifname_len; FILE *fp; char line[256], *colon; @@ -64,7 +64,7 @@ virNetInterfaceStats(const char *path, return -1; } - path_len = strlen(path); + ifname_len = strlen(ifname); while (fgets(line, sizeof(line), fp)) { long long dummy; @@ -84,8 +84,8 @@ virNetInterfaceStats(const char *path, colon = strchr(line, ':'); if (!colon) continue; *colon = '\0'; - if (colon-path_len >= line && - STREQ(colon-path_len, path)) { + if (colon-ifname_len >= line && + STREQ(colon-ifname_len, ifname)) { /* IMPORTANT NOTE! * /proc/net/dev vif<domid>.nn sees the network from the point * of view of dom0 / hypervisor. So bytes TRANSMITTED by dom0 @@ -121,8 +121,8 @@ virNetInterfaceStats(const char *path, } #elif defined(HAVE_GETIFADDRS) && defined(AF_LINK) int -virNetInterfaceStats(const char *path, - virDomainInterfaceStatsPtr stats) +virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) { struct ifaddrs *ifap, *ifa; struct if_data *ifd; @@ -138,7 +138,7 @@ virNetInterfaceStats(const char *path, if (ifa->ifa_addr->sa_family != AF_LINK) continue; - if (STREQ(ifa->ifa_name, path)) { + if (STREQ(ifa->ifa_name, ifname)) { ifd = (struct if_data *)ifa->ifa_data; stats->tx_bytes = ifd->ifi_ibytes; stats->tx_packets = ifd->ifi_ipackets; @@ -167,8 +167,8 @@ virNetInterfaceStats(const char *path, } #else int -virNetInterfaceStats(const char *path ATTRIBUTE_UNUSED, - virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED) +virNetDevTapInterfaceStats(const char *ifname ATTRIBUTE_UNUSED, + virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("interface stats not implemented on this platform")); diff --git a/src/util/virstats.h b/src/util/virstats.h index 69f4cf1..5b77197 100644 --- a/src/util/virstats.h +++ b/src/util/virstats.h @@ -25,7 +25,8 @@ # include "internal.h" -int virNetInterfaceStats(const char *path, - virDomainInterfaceStatsPtr stats); +int virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; #endif /* __STATS_LINUX_H__ */ diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 0177f83..efe5a8f 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1466,7 +1466,7 @@ xenHypervisorDomainInterfaceStats(virDomainDefPtr def, return -1; } - return virNetInterfaceStats(path, stats); + return virNetDevTapInterfaceStats(path, stats); #else virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("/proc/net/dev: Interface not found")); -- 2.10.2
On 18.11.2016 23:51, Mehdi Abaakouk wrote:
From: Mehdi Abaakouk <sileht@redhat.com>
In preparation to the code move to virnetdevtap.c, this change:
* renames virNetInterfaceStats to virNetDevTapInterfaceStats * changes 'path' to 'ifname', to use the same vocable as other method in virnetdevtap.c. * Add the attributes checker --- src/libvirt_private.syms | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/util/virstats.c | 22 +++++++++++----------- src/util/virstats.h | 5 +++-- src/xen/xen_hypervisor.c | 2 +- 8 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa27f78..0036cbd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2367,7 +2367,7 @@ virSocketAddrSetIPv6AddrNetOrder; virSocketAddrSetPort;
# util/virstats.h -virNetInterfaceStats; +virNetDevTapInterfaceStats;
# util/virstorageencryption.h virStorageEncryptionFormat; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b2f3b16..67f0e58 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4982,7 +4982,7 @@ libxlDomainInterfaceStats(virDomainPtr dom, }
if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("'%s' is not a known interface"), path); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 4a0165a..526d40d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2893,7 +2893,7 @@ lxcDomainInterfaceStats(virDomainPtr dom, }
if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("Invalid path, '%s' is not a known interface"), path); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 38a562e..7bd3acf 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2024,7 +2024,7 @@ openvzDomainInterfaceStats(virDomainPtr dom, }
if (ret == 0) - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); else virReportError(VIR_ERR_INVALID_ARG, _("invalid path, '%s' is not a known interface"), path); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87ca09d..38208b1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11005,7 +11005,7 @@ qemuDomainInterfaceStats(virDomainPtr dom, if (net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { ret = virNetDevOpenvswitchInterfaceStats(path, stats); } else { - ret = virNetInterfaceStats(path, stats); + ret = virNetDevTapInterfaceStats(path, stats); } } else { virReportError(VIR_ERR_INVALID_ARG, @@ -19154,7 +19154,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, continue; } } else { - if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { + if (virNetDevTapInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { virResetLastError(); continue; } diff --git a/src/util/virstats.c b/src/util/virstats.c index c4725ed..95b4c38 100644 --- a/src/util/virstats.c +++ b/src/util/virstats.c @@ -50,10 +50,10 @@ */ #ifdef __linux__ int -virNetInterfaceStats(const char *path, - virDomainInterfaceStatsPtr stats) +virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) { - int path_len; + int ifname_len; FILE *fp; char line[256], *colon;
@@ -64,7 +64,7 @@ virNetInterfaceStats(const char *path, return -1; }
- path_len = strlen(path); + ifname_len = strlen(ifname);
while (fgets(line, sizeof(line), fp)) { long long dummy; @@ -84,8 +84,8 @@ virNetInterfaceStats(const char *path, colon = strchr(line, ':'); if (!colon) continue; *colon = '\0'; - if (colon-path_len >= line && - STREQ(colon-path_len, path)) { + if (colon-ifname_len >= line && + STREQ(colon-ifname_len, ifname)) {
While touching this you can fix the spaces around '-' sign.
/* IMPORTANT NOTE! * /proc/net/dev vif<domid>.nn sees the network from the point * of view of dom0 / hypervisor. So bytes TRANSMITTED by dom0
ACK Michal
From: Mehdi Abaakouk <sileht@redhat.com> This is just a code move of virstat.c to virnetdevtap.c --- po/POTFILES.in | 1 - src/Makefile.am | 1 - src/libvirt_private.syms | 4 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 1 - src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/uml/uml_driver.c | 1 - src/util/virnetdevtap.c | 143 ++++++++++++++++++++++++++++++++++++ src/util/virnetdevtap.h | 3 + src/util/virstats.c | 178 --------------------------------------------- src/util/virstats.h | 32 -------- src/xen/xen_hypervisor.c | 2 +- 13 files changed, 151 insertions(+), 221 deletions(-) delete mode 100644 src/util/virstats.c delete mode 100644 src/util/virstats.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 25867ae..50fff6b 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -240,7 +240,6 @@ src/util/virscsi.c src/util/virsecret.c src/util/virsexpr.c src/util/virsocketaddr.c -src/util/virstats.c src/util/virstorageencryption.c src/util/virstoragefile.c src/util/virstring.c diff --git a/src/Makefile.am b/src/Makefile.am index aaba9e6..9c958aa 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -167,7 +167,6 @@ UTIL_SOURCES = \ util/virsecret.c util/virsecret.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ - util/virstats.c util/virstats.h \ util/virstorageencryption.c util/virstorageencryption.h \ util/virstoragefile.c util/virstoragefile.h \ util/virstring.h util/virstring.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0036cbd..9fd5920 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2045,6 +2045,7 @@ virNetDevTapCreateInBridgePort; virNetDevTapDelete; virNetDevTapGetName; virNetDevTapGetRealDeviceName; +virNetDevTapInterfaceStats; # util/virnetdevveth.h @@ -2366,9 +2367,6 @@ virSocketAddrSetIPv6Addr; virSocketAddrSetIPv6AddrNetOrder; virSocketAddrSetPort; -# util/virstats.h -virNetDevTapInterfaceStats; - # util/virstorageencryption.h virStorageEncryptionFormat; virStorageEncryptionFree; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 67f0e58..9454337 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -61,7 +61,7 @@ #include "virhostdev.h" #include "network/bridge_driver.h" #include "locking/domain_lock.h" -#include "virstats.h" +#include "virnetdevtap.h" #include "cpu/cpu.h" #define VIR_FROM_THIS VIR_FROM_LIBXL diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 526d40d..7a13c23 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -61,7 +61,6 @@ #include "virhostcpu.h" #include "virhostmem.h" #include "viruuid.h" -#include "virstats.h" #include "virhook.h" #include "virfile.h" #include "virpidfile.h" diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 7bd3acf..5e549e00 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -57,7 +57,7 @@ #include "virlog.h" #include "vircommand.h" #include "viruri.h" -#include "virstats.h" +#include "virnetdevtap.h" #include "virstring.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38208b1..1124429 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -65,7 +65,7 @@ #include "nodeinfo.h" #include "virhostcpu.h" #include "virhostmem.h" -#include "virstats.h" +#include "virnetdevtap.h" #include "virnetdevopenvswitch.h" #include "capabilities.h" #include "viralloc.h" diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 4f4a69b..d3fb08a 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -48,7 +48,6 @@ #include "nodeinfo.h" #include "virhostcpu.h" #include "virhostmem.h" -#include "virstats.h" #include "capabilities.h" #include "viralloc.h" #include "viruuid.h" diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 7488a4c..85c0045 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -33,7 +33,13 @@ #include "viralloc.h" #include "virlog.h" #include "virstring.h" +#include "datatypes.h" +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <regex.h> #include <dirent.h> #include <sys/types.h> #include <sys/ioctl.h> @@ -44,6 +50,9 @@ #elif defined(__FreeBSD__) # include <net/if_tap.h> #endif +#if defined(HAVE_GETIFADDRS) && defined(AF_LINK) +# include <ifaddrs.h> +#endif #define VIR_FROM_THIS VIR_FROM_NONE @@ -601,3 +610,137 @@ int virNetDevTapCreateInBridgePort(const char *brname, return -1; } + +/*-------------------- interface stats --------------------*/ +/* Just reads the named interface, so not Xen or QEMU-specific. + * NB. Caller must check that libvirt user is trying to query + * the interface of a domain they own. We do no such checking. + */ +#ifdef __linux__ +int +virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) +{ + int ifname_len; + FILE *fp; + char line[256], *colon; + + fp = fopen("/proc/net/dev", "r"); + if (!fp) { + virReportSystemError(errno, "%s", + _("Could not open /proc/net/dev")); + return -1; + } + + ifname_len = strlen(ifname); + + while (fgets(line, sizeof(line), fp)) { + long long dummy; + long long rx_bytes; + long long rx_packets; + long long rx_errs; + long long rx_drop; + long long tx_bytes; + long long tx_packets; + long long tx_errs; + long long tx_drop; + + /* The line looks like: + * " eth0:..." + * Split it at the colon. + */ + colon = strchr(line, ':'); + if (!colon) continue; + *colon = '\0'; + if (colon-ifname_len >= line && + STREQ(colon-ifname_len, ifname)) { + /* IMPORTANT NOTE! + * /proc/net/dev vif<domid>.nn sees the network from the point + * of view of dom0 / hypervisor. So bytes TRANSMITTED by dom0 + * are bytes RECEIVED by the domain. That's why the TX/RX fields + * appear to be swapped here. + */ + if (sscanf(colon+1, + "%lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld", + &tx_bytes, &tx_packets, &tx_errs, &tx_drop, + &dummy, &dummy, &dummy, &dummy, + &rx_bytes, &rx_packets, &rx_errs, &rx_drop, + &dummy, &dummy, &dummy, &dummy) != 16) + continue; + + stats->rx_bytes = rx_bytes; + stats->rx_packets = rx_packets; + stats->rx_errs = rx_errs; + stats->rx_drop = rx_drop; + stats->tx_bytes = tx_bytes; + stats->tx_packets = tx_packets; + stats->tx_errs = tx_errs; + stats->tx_drop = tx_drop; + VIR_FORCE_FCLOSE(fp); + + return 0; + } + } + VIR_FORCE_FCLOSE(fp); + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("/proc/net/dev: Interface not found")); + return -1; +} +#elif defined(HAVE_GETIFADDRS) && defined(AF_LINK) +int +virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) +{ + struct ifaddrs *ifap, *ifa; + struct if_data *ifd; + int ret = -1; + + if (getifaddrs(&ifap) < 0) { + virReportSystemError(errno, "%s", + _("Could not get interface list")); + return -1; + } + + for (ifa = ifap; ifa; ifa = ifa->ifa_next) { + if (ifa->ifa_addr->sa_family != AF_LINK) + continue; + + if (STREQ(ifa->ifa_name, ifname)) { + ifd = (struct if_data *)ifa->ifa_data; + stats->tx_bytes = ifd->ifi_ibytes; + stats->tx_packets = ifd->ifi_ipackets; + stats->tx_errs = ifd->ifi_ierrors; + stats->tx_drop = ifd->ifi_iqdrops; + stats->rx_bytes = ifd->ifi_obytes; + stats->rx_packets = ifd->ifi_opackets; + stats->rx_errs = ifd->ifi_oerrors; +# ifdef HAVE_STRUCT_IF_DATA_IFI_OQDROPS + stats->rx_drop = ifd->ifi_oqdrops; +# else + stats->rx_drop = 0; +# endif + + ret = 0; + break; + } + } + + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Interface not found")); + + freeifaddrs(ifap); + return ret; +} +#else +int +virNetDevTapInterfaceStats(const char *ifname ATTRIBUTE_UNUSED, + virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("interface stats not implemented on this platform")); + return -1; +} + +#endif /* __linux__ */ diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index 20dec58..259e7c9 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -75,5 +75,8 @@ int virNetDevTapCreateInBridgePort(const char *brname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int virNetDevTapInterfaceStats(const char *ifname, + virDomainInterfaceStatsPtr stats) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_TAP_H__ */ diff --git a/src/util/virstats.c b/src/util/virstats.c deleted file mode 100644 index 95b4c38..0000000 --- a/src/util/virstats.c +++ /dev/null @@ -1,178 +0,0 @@ -/* - * virstats.c: Block and network stats. - * - * Copyright (C) 2007-2010, 2014 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - * Richard W.M. Jones <rjones@redhat.com> - */ - -#include <config.h> - -#include <stdio.h> -#include <stdlib.h> -#include <fcntl.h> -#include <string.h> -#include <unistd.h> -#include <regex.h> - -#if defined(HAVE_GETIFADDRS) && defined(AF_LINK) -# include <net/if.h> -# include <ifaddrs.h> -#endif - -#include "virerror.h" -#include "datatypes.h" -#include "virstats.h" -#include "viralloc.h" -#include "virfile.h" - -#define VIR_FROM_THIS VIR_FROM_STATS_LINUX - - -/*-------------------- interface stats --------------------*/ -/* Just reads the named interface, so not Xen or QEMU-specific. - * NB. Caller must check that libvirt user is trying to query - * the interface of a domain they own. We do no such checking. - */ -#ifdef __linux__ -int -virNetDevTapInterfaceStats(const char *ifname, - virDomainInterfaceStatsPtr stats) -{ - int ifname_len; - FILE *fp; - char line[256], *colon; - - fp = fopen("/proc/net/dev", "r"); - if (!fp) { - virReportSystemError(errno, "%s", - _("Could not open /proc/net/dev")); - return -1; - } - - ifname_len = strlen(ifname); - - while (fgets(line, sizeof(line), fp)) { - long long dummy; - long long rx_bytes; - long long rx_packets; - long long rx_errs; - long long rx_drop; - long long tx_bytes; - long long tx_packets; - long long tx_errs; - long long tx_drop; - - /* The line looks like: - * " eth0:..." - * Split it at the colon. - */ - colon = strchr(line, ':'); - if (!colon) continue; - *colon = '\0'; - if (colon-ifname_len >= line && - STREQ(colon-ifname_len, ifname)) { - /* IMPORTANT NOTE! - * /proc/net/dev vif<domid>.nn sees the network from the point - * of view of dom0 / hypervisor. So bytes TRANSMITTED by dom0 - * are bytes RECEIVED by the domain. That's why the TX/RX fields - * appear to be swapped here. - */ - if (sscanf(colon+1, - "%lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld", - &tx_bytes, &tx_packets, &tx_errs, &tx_drop, - &dummy, &dummy, &dummy, &dummy, - &rx_bytes, &rx_packets, &rx_errs, &rx_drop, - &dummy, &dummy, &dummy, &dummy) != 16) - continue; - - stats->rx_bytes = rx_bytes; - stats->rx_packets = rx_packets; - stats->rx_errs = rx_errs; - stats->rx_drop = rx_drop; - stats->tx_bytes = tx_bytes; - stats->tx_packets = tx_packets; - stats->tx_errs = tx_errs; - stats->tx_drop = tx_drop; - VIR_FORCE_FCLOSE(fp); - - return 0; - } - } - VIR_FORCE_FCLOSE(fp); - - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("/proc/net/dev: Interface not found")); - return -1; -} -#elif defined(HAVE_GETIFADDRS) && defined(AF_LINK) -int -virNetDevTapInterfaceStats(const char *ifname, - virDomainInterfaceStatsPtr stats) -{ - struct ifaddrs *ifap, *ifa; - struct if_data *ifd; - int ret = -1; - - if (getifaddrs(&ifap) < 0) { - virReportSystemError(errno, "%s", - _("Could not get interface list")); - return -1; - } - - for (ifa = ifap; ifa; ifa = ifa->ifa_next) { - if (ifa->ifa_addr->sa_family != AF_LINK) - continue; - - if (STREQ(ifa->ifa_name, ifname)) { - ifd = (struct if_data *)ifa->ifa_data; - stats->tx_bytes = ifd->ifi_ibytes; - stats->tx_packets = ifd->ifi_ipackets; - stats->tx_errs = ifd->ifi_ierrors; - stats->tx_drop = ifd->ifi_iqdrops; - stats->rx_bytes = ifd->ifi_obytes; - stats->rx_packets = ifd->ifi_opackets; - stats->rx_errs = ifd->ifi_oerrors; -# ifdef HAVE_STRUCT_IF_DATA_IFI_OQDROPS - stats->rx_drop = ifd->ifi_oqdrops; -# else - stats->rx_drop = 0; -# endif - - ret = 0; - break; - } - } - - if (ret < 0) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Interface not found")); - - freeifaddrs(ifap); - return ret; -} -#else -int -virNetDevTapInterfaceStats(const char *ifname ATTRIBUTE_UNUSED, - virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED) -{ - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("interface stats not implemented on this platform")); - return -1; -} - -#endif /* __linux__ */ diff --git a/src/util/virstats.h b/src/util/virstats.h deleted file mode 100644 index 5b77197..0000000 --- a/src/util/virstats.h +++ /dev/null @@ -1,32 +0,0 @@ -/* - * virstats.h: Block and network stats. - * - * Copyright (C) 2007 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library. If not, see - * <http://www.gnu.org/licenses/>. - * - * Richard W.M. Jones <rjones@redhat.com> - */ - -#ifndef __STATS_LINUX_H__ -# define __STATS_LINUX_H__ - -# include "internal.h" - -int virNetDevTapInterfaceStats(const char *ifname, - virDomainInterfaceStatsPtr stats) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; - -#endif /* __STATS_LINUX_H__ */ diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index efe5a8f..bce7b56 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -68,7 +68,7 @@ #include "xen_driver.h" #include "xen_hypervisor.h" #include "xs_internal.h" -#include "virstats.h" +#include "virnetdevtap.h" #include "block_stats.h" #include "xend_internal.h" #include "virbuffer.h" -- 2.10.2
On 18.11.2016 23:51, Mehdi Abaakouk wrote:
From: Mehdi Abaakouk <sileht@redhat.com>
This is just a code move of virstat.c to virnetdevtap.c --- po/POTFILES.in | 1 - src/Makefile.am | 1 - src/libvirt_private.syms | 4 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 1 - src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/uml/uml_driver.c | 1 - src/util/virnetdevtap.c | 143 ++++++++++++++++++++++++++++++++++++ src/util/virnetdevtap.h | 3 + src/util/virstats.c | 178 --------------------------------------------- src/util/virstats.h | 32 -------- src/xen/xen_hypervisor.c | 2 +- 13 files changed, 151 insertions(+), 221 deletions(-) delete mode 100644 src/util/virstats.c delete mode 100644 src/util/virstats.h
ACK Michal
From: Mehdi Abaakouk <sileht@redhat.com> This change puts the socket filename as ifname for vhostuser netwok interface. The filename is the name of the openvswitch interface, this allows the qemuDomainInterfaceStats to work out of the box without having to manually set the ifname. --- src/conf/domain_conf.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..0f91ab3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9098,6 +9098,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; int ret, val; + char **tokens = NULL; + size_t ntokens = 0; if (VIR_ALLOC(def) < 0) return NULL; @@ -9394,6 +9396,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX; def->data.vhostuser->data.nix.path = vhostuser_path; + + if (!ifname && (tokens = virStringSplitCount(vhostuser_path, "/", 0, + &ntokens))) { + if (VIR_STRDUP(ifname, tokens[ntokens-1]) < 0) + goto error; + } + vhostuser_path = NULL; if (STREQ(vhostuser_mode, "server")) { @@ -9842,6 +9851,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(localaddr); VIR_FREE(localport); virNWFilterHashTableFree(filterparams); + virStringFreeListCount(tokens, ntokens); return def; -- 2.10.2
On 18.11.2016 23:51, Mehdi Abaakouk wrote:
From: Mehdi Abaakouk <sileht@redhat.com>
This change puts the socket filename as ifname for vhostuser netwok interface.
The filename is the name of the openvswitch interface, this allows the qemuDomainInterfaceStats to work out of the box without having to manually set the ifname. --- src/conf/domain_conf.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..0f91ab3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9098,6 +9098,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; int ret, val; + char **tokens = NULL; + size_t ntokens = 0;
if (VIR_ALLOC(def) < 0) return NULL; @@ -9394,6 +9396,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX; def->data.vhostuser->data.nix.path = vhostuser_path; + + if (!ifname && (tokens = virStringSplitCount(vhostuser_path, "/", 0, + &ntokens))) { + if (VIR_STRDUP(ifname, tokens[ntokens-1]) < 0) + goto error; + } +
I don't think this is a good idea. For instance, for the following XML: <interface type='vhostuser'> <mac address='52:54:00:ee:96:6b'/> <source type='unix' path='/tmp/vhost0.sock' mode='server'/> <model type='virtio'/> </interface> this code would produce ifname of "vhost0.sock", which is obviously wrong. Moreover, tests are failing with this change. Not only that, for auto-filling values in XML we have so called post parse callbacks. Historically we put everything into XML parsing code and it ended up being this one big mess. So we worked hard to split it and although we are not there 100%, we are getting there slowly.
vhostuser_path = NULL;
if (STREQ(vhostuser_mode, "server")) { @@ -9842,6 +9851,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(localaddr); VIR_FREE(localport); virNWFilterHashTableFree(filterparams); + virStringFreeListCount(tokens, ntokens);
Ah, due to me not reviewing the patches early, this function was renamed in c2a5a4e7ea930.
return def;
Michal
On Thu, Dec 08, 2016 at 01:51:10PM +0100, Michal Privoznik wrote:
On 18.11.2016 23:51, Mehdi Abaakouk wrote:
From: Mehdi Abaakouk <sileht@redhat.com>
I don't think this is a good idea. For instance, for the following XML:
<interface type='vhostuser'> <mac address='52:54:00:ee:96:6b'/> <source type='unix' path='/tmp/vhost0.sock' mode='server'/> <model type='virtio'/> </interface>
this code would produce ifname of "vhost0.sock", which is obviously wrong. Moreover, tests are failing with this change. Not only that, for auto-filling values in XML we have so called post parse callbacks. Historically we put everything into XML parsing code and it ended up being this one big mess. So we worked hard to split it and although we are not there 100%, we are getting there slowly.
I agree I have proposed that to start a discussion, but I don't really like it. It work in my case because openvswitch vhostuser unix-socket path is hardcoded to /var/run/openvswitch/<ovs-interface-name>. But if you vhostuser unix-socket have been created outside openvswitch that obviously doesn't pick a good name, and it's even not really useful to set a default name. Perhaps should I move this logic to virnetdevopenvswitch.c. I means virNetDevOpenvswitchInterfaceStats() should take a look to the source path when the ifname is unset, WDT ?
vhostuser_path = NULL;
if (STREQ(vhostuser_mode, "server")) { @@ -9842,6 +9851,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(localaddr); VIR_FREE(localport); virNWFilterHashTableFree(filterparams); + virStringFreeListCount(tokens, ntokens);
Ah, due to me not reviewing the patches early, this function was renamed in c2a5a4e7ea930.
return def;
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
Hi, Can I have any feedback of this ? Thank in advance. Le 2016-11-18 23:51, Mehdi Abaakouk a écrit :
This new version removes the virstat.c from locale configuration.
It also adds a new commit to autodected the ifname of the ovs vhostuser.
Mehdi Abaakouk (4): Gathering vhostuser interface stats with ovs virstat: fix signature of virstat helper Move virstat.c code to virnetdevtap.c domain_conf: autodetect vhostuser ifname
po/POTFILES.in | 1 - src/Makefile.am | 1 - src/conf/domain_conf.c | 10 +++ src/libvirt_private.syms | 5 +- src/libxl/libxl_driver.c | 4 +- src/lxc/lxc_driver.c | 3 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_driver.c | 31 +++++-- src/uml/uml_driver.c | 1 - src/util/virnetdevopenvswitch.c | 106 ++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 + src/util/virnetdevtap.c | 143 ++++++++++++++++++++++++++++++++ src/util/virnetdevtap.h | 3 + src/util/virstats.c | 178 ---------------------------------------- src/util/virstats.h | 31 ------- src/xen/xen_hypervisor.c | 4 +- 16 files changed, 298 insertions(+), 231 deletions(-) delete mode 100644 src/util/virstats.c delete mode 100644 src/util/virstats.h
-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
Hi, Sorry to bother you again, but I still have no comment about using openvswitch statistics command to retrieve vhost-user interface statistics. Regards, On Fri, Nov 18, 2016 at 11:51:12PM +0100, Mehdi Abaakouk wrote:
This new version removes the virstat.c from locale configuration.
It also adds a new commit to autodected the ifname of the ovs vhostuser.
Mehdi Abaakouk (4): Gathering vhostuser interface stats with ovs virstat: fix signature of virstat helper Move virstat.c code to virnetdevtap.c domain_conf: autodetect vhostuser ifname
po/POTFILES.in | 1 - src/Makefile.am | 1 - src/conf/domain_conf.c | 10 +++ src/libvirt_private.syms | 5 +- src/libxl/libxl_driver.c | 4 +- src/lxc/lxc_driver.c | 3 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_driver.c | 31 +++++-- src/uml/uml_driver.c | 1 - src/util/virnetdevopenvswitch.c | 106 ++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 4 + src/util/virnetdevtap.c | 143 ++++++++++++++++++++++++++++++++ src/util/virnetdevtap.h | 3 + src/util/virstats.c | 178 ---------------------------------------- src/util/virstats.h | 31 ------- src/xen/xen_hypervisor.c | 4 +- 16 files changed, 298 insertions(+), 231 deletions(-) delete mode 100644 src/util/virstats.c delete mode 100644 src/util/virstats.h
-- 2.10.2
-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht
participants (2)
-
Mehdi Abaakouk -
Michal Privoznik