src/util/virnetdevopenvswitch.c | 96 +++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 62 deletions(-)
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@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
@@ -317,14 +317,8 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
{
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;
+ char *tmp;
+ bool gotStats = false;
int ret = -1;
/* Just ensure the interface exists in ovs */
@@ -340,67 +334,45 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
goto cleanup;
}
- VIR_FREE(output);
- virCommandFree(cmd);
-
- cmd = virCommandNew(OVSVSCTL);
- virNetDevOpenvswitchAddTimeout(cmd);
- virCommandAddArgList(cmd, "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;
- }
+#define GET_STAT(name, member) \
+ do { \
+ VIR_FREE(output); \
+ virCommandFree(cmd); \
+ cmd = virCommandNew(OVSVSCTL); \
+ virNetDevOpenvswitchAddTimeout(cmd); \
+ virCommandAddArgList(cmd, "get", "Interface", ifname, \
+ "statistics:" name, NULL); \
+ virCommandSetOutputBuffer(cmd, &output); \
+ if (virCommandRun(cmd, NULL) < 0) { \
+ stats->member = -1; \
+ } else { \
+ if (virStrToLong_ll(output, &tmp, 10, &stats->member) < 0 || \
+ *tmp != '\n') { \
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
+ _("Fail to parse ovs-vsctl output")); \
+ goto cleanup; \
+ } \
+ gotStats = true; \
+ } \
+ } while (0)
/* 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;
- }
+ GET_STAT("rx_bytes", tx_bytes);
+ GET_STAT("rx_packets", tx_packets);
+ GET_STAT("rx_errors", tx_errs);
+ GET_STAT("rx_dropped", tx_drop);
+ GET_STAT("tx_bytes", rx_bytes);
+ GET_STAT("tx_packets", rx_packets);
+ GET_STAT("tx_errors", rx_errs);
+ GET_STAT("tx_dropped", rx_drop);
- 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 = virCommandNew(OVSVSCTL);
- virNetDevOpenvswitchAddTimeout(cmd);
- virCommandAddArgList(cmd, "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 {
+ if (!gotStats) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Fail to parse ovs-vsctl output"));
+ _("Interface doesn't have statistics"));
goto cleanup;
}
+
ret = 0;
cleanup:
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/23/2017 11:32 AM, Martin Kletzander wrote:
> 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@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)?
Well, if it doesn't return a non-zero value on an error, it's ovs bug
because of their documentation:
EXIT STATUS
0 Successful program execution.
1 Usage, syntax, or configuration file error.
>
> Anyway, your approach is more error-prone, even though we're running the
> command 8 times (ewww).
Yes, I don't like it either.
> 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?),
Good idea, I'll add it there.
> 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.
Well, the ultimate goal would be to communicate with ovs directly via
socket instead of spawning ovs-vsctl command repeatedly (which is
something we do in other areas of the code btw - e.g. when plugging a
nic into an ovs bridge). However, that is a long shot from here, because
so far OVS devels consider their protocol the same way we do - internal
implementation. There is no stable library for communicating with ovs
yet. So I'm afraid for now we're stuck with spawning binaries :(
>
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks, pushed.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.