Re: [PATCH] virNetDevOpenvswitchInterfaceStats: Be more forgiving when fetching stats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 22, 2017 at 03:44:46PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1461270

When fetching stats for a vhost-user type of interface, we run
couple of ovs-vsctl commands and parse their output. However, not
all stats exist at all times, for instance "rx_dropped" or
"tx_errors" can be missing. Thing is, we ask for a bulk of
statistics and if one of them is missing an error is reported
instead of returning the rest. Since we ignore errors, we fail to
set statistics. Fix this by asking for each piece alone.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/util/virnetdevopenvswitch.c | 96 +++++++++++++++--------------------------
1 file changed, 34 insertions(+), 62 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 8f7215e06..6848f65b7 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -340,67 +334,45 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,

[...]

+    if (!gotStats) {
        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Fail to parse ovs-vsctl output"));
+                       _("Interface doesn't have statistics"));

s/have/have any/ or s/have/support/

        goto cleanup;

Is it possible that the version of ovs-vsctl command doesn't return
non-zero value and the right fix is just setting the values to -1
instead of the 'goto cleanup;' here (and leave it to fail if one of the
first four values are missing)?

Anyway, your approach is more error-prone, even though we're running the
command 8 times (ewww).  But the output of `ovs-vsctl get interface
$ifname statistics` is not in a format that we would have parsers for.
Even though it wouldn't be that hard to parse it (wiki:BiteSizedTasks?),
it's not that big of a deal except the constant statistics-gathering
APIs.  But it would be nice if we fixed that in the (hopefully near) future.

Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux