We run 'ovs-vsctl' nine times (first to find if interface is
there and then eight times = for each stats member separately).
This is very inefficient. I've found a way to run it once and
with a bit of help from virJSON module we can parse out stats
we need.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virnetdevopenvswitch.c | 111 +++++++++++++++++++++-----------
1 file changed, 74 insertions(+), 37 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index c99ecfbf15..0fe64bedab 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -28,6 +28,7 @@
#include "virmacaddr.h"
#include "virstring.h"
#include "virlog.h"
+#include "virjson.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -311,58 +312,94 @@ int
virNetDevOpenvswitchInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats)
{
- char *tmp;
- bool gotStats = false;
VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOFREE(char *) output = NULL;
+ VIR_AUTOPTR(virJSONValue) jsonStats = NULL;
+ virJSONValuePtr jsonMap = NULL;
+ size_t i;
- /* Just ensure the interface exists in ovs */
cmd = virCommandNew(OVSVSCTL);
virNetDevOpenvswitchAddTimeout(cmd);
- virCommandAddArgList(cmd, "get", "Interface", ifname, "name", NULL);
+ virCommandAddArgList(cmd, "--if-exists", "--format=list", "--data=json",
+ "--no-headings", "--columns=statistics", "list",
+ "Interface", ifname, NULL);
virCommandSetOutputBuffer(cmd, &output);
- if (virCommandRun(cmd, NULL) < 0) {
+ /* The above command returns either:
+ * 1) empty string if @ifname doesn't exist, or
+ * 2) a JSON array, for instance:
+ * ["map",[["collisions",0],["rx_bytes",0],["rx_crc_err",0],["rx_dropped",0],
+ * ["rx_errors",0],["rx_frame_err",0],["rx_over_err",0],["rx_packets",0],
+ * ["tx_bytes",12406],["tx_dropped",0],["tx_errors",0],["tx_packets",173]]]
+ */
+
+ if (virCommandRun(cmd, NULL) < 0 ||
+ STREQ_NULLABLE(output, "")) {
/* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Interface not found"));
return -1;
}
-#define GET_STAT(name, member) \
- do { \
- VIR_FREE(output); \
- virCommandFree(cmd); \
- cmd = virCommandNew(OVSVSCTL); \
- virNetDevOpenvswitchAddTimeout(cmd); \
- virCommandAddArgList(cmd, "--if-exists", "get", "Interface", \
- ifname, "statistics:" name, NULL); \
- virCommandSetOutputBuffer(cmd, &output); \
- if (virCommandRun(cmd, NULL) < 0 || !output || !*output || *output == '\n') { \
- 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")); \
- return -1; \
- } \
- gotStats = true; \
- } \
- } while (0)
+ if (!(jsonStats = virJSONValueFromString(output)) ||
+ !virJSONValueIsArray(jsonStats) ||
+ !(jsonMap = virJSONValueArrayGet(jsonStats, 1))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to parse ovs-vsctl output"));
+ return -1;
+ }
- /* The TX/RX fields appear to be swapped here
- * because this is the host view. */
- 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 = stats->rx_packets = stats->rx_errs = stats->rx_drop = -1;
+ stats->tx_bytes = stats->tx_packets = stats->tx_errs = stats->tx_drop = -1;
- if (!gotStats) {
+ for (i = 0; i < virJSONValueArraySize(jsonMap); i++) {
+ virJSONValuePtr item = virJSONValueArrayGet(jsonMap, i);
+ virJSONValuePtr jsonKey;
+ virJSONValuePtr jsonVal;
+ const char *key;
+ long long val;
+
+ if (!item ||
+ (!(jsonKey = virJSONValueArrayGet(item, 0))) ||
+ (!(jsonVal = virJSONValueArrayGet(item, 1))) ||
+ (!(key = virJSONValueGetString(jsonKey))) ||
+ (virJSONValueGetNumberLong(jsonVal, &val) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Malformed ovs-vsctl output"));
+ return -1;
+ }
+
+ /* The TX/RX fields appear to be swapped here
+ * because this is the host view. */
+ if (STREQ(key, "rx_bytes")) {
+ stats->tx_bytes = val;
+ } else if (STREQ(key, "rx_packets")) {
+ stats->tx_packets = val;
+ } else if (STREQ(key, "rx_errors")) {
+ stats->tx_errs = val;
+ } else if (STREQ(key, "rx_dropped")) {
+ stats->tx_drop = val;
+ } else if (STREQ(key, "tx_bytes")) {
+ stats->rx_bytes = val;
+ } else if (STREQ(key, "tx_packets")) {
+ stats->rx_packets = val;
+ } else if (STREQ(key, "tx_errors")) {
+ stats->rx_errs = val;
+ } else if (STREQ(key, "tx_dropped")) {
+ stats->rx_drop = val;
+ } else {
+ VIR_DEBUG("Unused ovs-vsctl stat key=%s val=%lld", key, val);
+ }
+ }
+
+ if (stats->rx_bytes == -1 &&
+ stats->rx_packets == -1 &&
+ stats->rx_errs == -1 &&
+ stats->rx_drop == -1 &&
+ stats->tx_bytes == -1 &&
+ stats->tx_packets == -1 &&
+ stats->tx_errs == -1 &&
+ stats->tx_drop == -1) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Interface doesn't have any statistics"));
return -1;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 03, 2019 at 09:19:18AM +0200, Michal Privoznik wrote:
>We run 'ovs-vsctl' nine times (first to find if interface is
>there and then eight times = for each stats member separately).
>This is very inefficient. I've found a way to run it once and
>with a bit of help from virJSON module we can parse out stats
>we need.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/util/virnetdevopenvswitch.c | 111 +++++++++++++++++++++-----------
> 1 file changed, 74 insertions(+), 37 deletions(-)
>
>diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
>index c99ecfbf15..0fe64bedab 100644
>--- a/src/util/virnetdevopenvswitch.c
>+++ b/src/util/virnetdevopenvswitch.c
>@@ -28,6 +28,7 @@
> #include "virmacaddr.h"
> #include "virstring.h"
> #include "virlog.h"
>+#include "virjson.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
>@@ -311,58 +312,94 @@ int
> virNetDevOpenvswitchInterfaceStats(const char *ifname,
> virDomainInterfaceStatsPtr stats)
> {
>- char *tmp;
>- bool gotStats = false;
> VIR_AUTOPTR(virCommand) cmd = NULL;
> VIR_AUTOFREE(char *) output = NULL;
>+ VIR_AUTOPTR(virJSONValue) jsonStats = NULL;
>+ virJSONValuePtr jsonMap = NULL;
>+ size_t i;
>
>- /* Just ensure the interface exists in ovs */
> cmd = virCommandNew(OVSVSCTL);
> virNetDevOpenvswitchAddTimeout(cmd);
>- virCommandAddArgList(cmd, "get", "Interface", ifname, "name", NULL);
>+ virCommandAddArgList(cmd, "--if-exists", "--format=list", "--data=json",
>+ "--no-headings", "--columns=statistics", "list",
>+ "Interface", ifname, NULL);
Can we assume these options are present in all the supported versions of
ovs-vsctl?
> virCommandSetOutputBuffer(cmd, &output);
>
>- if (virCommandRun(cmd, NULL) < 0) {
>+ /* The above command returns either:
>+ * 1) empty string if @ifname doesn't exist, or
>+ * 2) a JSON array, for instance:
>+ * ["map",[["collisions",0],["rx_bytes",0],["rx_crc_err",0],["rx_dropped",0],
>+ * ["rx_errors",0],["rx_frame_err",0],["rx_over_err",0],["rx_packets",0],
>+ * ["tx_bytes",12406],["tx_dropped",0],["tx_errors",0],["tx_packets",173]]]
>+ */
>+
This example would look better in tests/ where you test this function
against actual output O:-)
>+ if (virCommandRun(cmd, NULL) < 0 ||
>+ STREQ_NULLABLE(output, "")) {
> /* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Interface not found"));
> return -1;
> }
>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 7/12/19 6:28 PM, Ján Tomko wrote:
> On Wed, Jul 03, 2019 at 09:19:18AM +0200, Michal Privoznik wrote:
>> We run 'ovs-vsctl' nine times (first to find if interface is
>> there and then eight times = for each stats member separately).
>> This is very inefficient. I've found a way to run it once and
>> with a bit of help from virJSON module we can parse out stats
>> we need.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/util/virnetdevopenvswitch.c | 111 +++++++++++++++++++++-----------
>> 1 file changed, 74 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/util/virnetdevopenvswitch.c
>> b/src/util/virnetdevopenvswitch.c
>> index c99ecfbf15..0fe64bedab 100644
>> --- a/src/util/virnetdevopenvswitch.c
>> +++ b/src/util/virnetdevopenvswitch.c
>> @@ -28,6 +28,7 @@
>> #include "virmacaddr.h"
>> #include "virstring.h"
>> #include "virlog.h"
>> +#include "virjson.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_NONE
>>
>> @@ -311,58 +312,94 @@ int
>> virNetDevOpenvswitchInterfaceStats(const char *ifname,
>> virDomainInterfaceStatsPtr stats)
>> {
>> - char *tmp;
>> - bool gotStats = false;
>> VIR_AUTOPTR(virCommand) cmd = NULL;
>> VIR_AUTOFREE(char *) output = NULL;
>> + VIR_AUTOPTR(virJSONValue) jsonStats = NULL;
>> + virJSONValuePtr jsonMap = NULL;
>> + size_t i;
>>
>> - /* Just ensure the interface exists in ovs */
>> cmd = virCommandNew(OVSVSCTL);
>> virNetDevOpenvswitchAddTimeout(cmd);
>> - virCommandAddArgList(cmd, "get", "Interface", ifname, "name", NULL);
>> + virCommandAddArgList(cmd, "--if-exists", "--format=list",
>> "--data=json",
>> + "--no-headings", "--columns=statistics",
>> "list",
>> + "Interface", ifname, NULL);
>
> Can we assume these options are present in all the supported versions of
> ovs-vsctl?
Rough skim through ovs' git log shows that these were added starting
from v1.1.0pre1~233 till v1.1.0~294 (mid of 2010 - beginnig of 2011).
Therefore, I believe it's safe to use them. And if there's somebody
running 10 years old ovs with the latest libvirt we can tell them to update.
>
>> virCommandSetOutputBuffer(cmd, &output);
>>
>> - if (virCommandRun(cmd, NULL) < 0) {
>> + /* The above command returns either:
>> + * 1) empty string if @ifname doesn't exist, or
>> + * 2) a JSON array, for instance:
>> + *
>> ["map",[["collisions",0],["rx_bytes",0],["rx_crc_err",0],["rx_dropped",0],
>>
>> + *
>> ["rx_errors",0],["rx_frame_err",0],["rx_over_err",0],["rx_packets",0],
>> + *
>> ["tx_bytes",12406],["tx_dropped",0],["tx_errors",0],["tx_packets",173]]]
>> + */
>> +
>
> This example would look better in tests/ where you test this function
> against actual output O:-)
Well, we don't have virNetDevOpenvswitch test or anything. But even if
we did, the test case would work over the output we obtained at some
point in the time. The input would not change. So effectively, we would
be testing only our JSON parsing skills and not if the function fetches
the correct data. Sorry.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.