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@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)? 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@xxxxxxxxxx> Thanks, pushed. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
