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

[Martin Kletzander]

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 at redhat.com>
>---
> 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 at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170623/ef287410/attachment-0001.sig>