9:54 a.m.
$subj:
util: Introduce virNetDevGetProcNetdevStats
On 06/28/2018 09:22 AM, Jai Singh Rana wrote:
This patch introduces virNetDevGetProcNetdevStats API for linux
which returns stats for the given interface from /proc/net/dev by
properly mapping stats entries in probed column to column header.
in the probed column...
This new method is a rewrite of the existing virNetDevTapInterfaceStats
in order to perform stricter data/column consistency checks between the
output from the file and the libvirt expected data/columns.
Tap and hostdev network devices will now be using this API.
Currently function virNetDevTapInterfaceStats which earlier fetched
stats for tap devies is modified to use above API accordingly.
Replace above paragraph with:
Since the new API is better than the existing
virNetDevTapInterfaceStats, the former API will just call the new API.
Separating the API out also makes it possible to reuse for both the
existing tap statistics and in the future patches for hostdev network
devices.
Signed-off-by: Jai Singh Rana <JaiSingh.Rana(a)cavium.com>
---
src/libvirt_private.syms | 1 +
src/util/virnetdev.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++-
src/util/virnetdev.h | 5 ++
src/util/virnetdevtap.c | 71 +----------------
4 files changed, 208 insertions(+), 71 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 272e7426dd..0ba8cd2a14 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2289,6 +2289,7 @@ virNetDevGetName;
virNetDevGetOnline;
virNetDevGetPhysicalFunction;
virNetDevGetPhysPortID;
+virNetDevGetProcNetdevStats;
virNetDevGetPromiscuous;
virNetDevGetRcvAllMulti;
virNetDevGetRcvMulti;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index b250af9e2c..69875c2e04 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1541,6 +1541,198 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char
**pfname,
return ret;
}
+
+/**
+ * virNetDevGetProcNetdevStats:
+ * @ifname: interface
+ * @stats: where to store statistics
+ * @swapped: whether to swap RX/TX fields
+ *
+ * Fetch RX/TX statistics for given named interface (@ifname) and
+ * store them at @stats. The returned statistics are always from
+ * domain POV. Because in some cases this means swapping RX/TX in
+ * the stats and in others this means no swapping (consider TAP
+ * vs macvtap) caller might choose if the returned stats should
+ * be @swapped or not.
+ *
+ * Returns 0 on success, -1 otherwise (with error reported).
+ */
+
+int
+virNetDevGetProcNetdevStats(const char *ifname,
+ virDomainInterfaceStatsPtr stats,
+ bool swapped)
+{
+ int ifname_len;
+ FILE *fp;
+ char line[256];
+ size_t i;
+ char *hdr1 = NULL;
+ char *hdr2 = NULL;
+ char *stats_row = NULL;
+ char **components_hdr1 = NULL;
+ char **components_hdr2 = NULL;
+ char **components_stats = NULL;
+ size_t ncomponents_hdr1 = 0;
+ size_t ncomponents_hdr2 = 0;
+ size_t ncomponents_stats = 0;
+ char **rx_hdr = NULL;
+ char **tx_hdr = NULL;
+ size_t rx_nentries, tx_nentries;
+ int rx_bytes_index = 0;
+ int rx_packets_index = 0;
+ int rx_drop_index = 0;
+ int rx_errs_index = 0;
+ int tx_bytes_index = 0;
+ int tx_packets_index = 0;
+ int tx_drop_index = 0;
+ int tx_errs_index = 0;
+ int tx_offset = 0;
+ int rx_offset = 0;
+ int ret = -1;
+
From 318d54e5
+ if (!ifname) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Interface name not provided"));
+ return -1;
+ }
+
+ fp = fopen("/proc/net/dev", "r");
+ if (!fp) {
+ virReportSystemError(errno, "%s",
+ _("Could not open /proc/net/dev"));
+ return ret;
return -1 is fine
+ }
+
+ ifname_len = strlen(ifname);
+
+ /* First two lines contains headers.
+ * Process headers to match with correspondings stats.
+ */
+ if (!fgets(line, sizeof(line), fp)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Interface %s not found in /proc/net/dev"), ifname);
+ VIR_FORCE_FCLOSE(fp);
+ return ret;
return -1 is fine, although going to cleanup is also possible.
+ }
+
+ if (!(hdr1 = virStringCleanExtraSpaces(line)))
+ goto cleanup;
Not sure the clean matters here since all you're doing is a STREQ later.
No reason a strstr couldn't be used later.
+
+ if (!(components_hdr1 = virStringSplitCount(hdr1, "|", 0,
+ &ncomponents_hdr1)))
+ goto cleanup;
at this point ncomponents_hdr should be == 3, right? May want to
check... and it had better be at least 2 since you reference [1] later.
+
+ if (!fgets(line, sizeof(line), fp))
+ goto cleanup;
+
+ if (!(hdr2 = virStringCleanExtraSpaces(line)))
+ goto cleanup;
+
+ if (!(components_hdr2 = virStringSplitCount(hdr2, "|", 0,
+ &ncomponents_hdr2)))
+ goto cleanup;
+
Likewise, ncomponents_hdr2 had better be 3 especially since you
reference [1] and [2] below...
+ if (!(rx_hdr = virStringSplitCount(components_hdr2[1], "
", 0,
+ &rx_nentries)))
+ goto cleanup;
+
+ if (!(tx_hdr = virStringSplitCount(components_hdr2[2], " ", 0,
+ &tx_nentries)))
+ goto cleanup;
+
+ if (STREQ(components_hdr1[1], "Receive")) {
+ rx_offset = 0;
+ tx_offset = rx_nentries;
+ } else {
+ rx_offset = tx_nentries;
+ tx_offset = 0;
+ }
+
+ for (i = 0; i < rx_nentries; i++) {
+ if (STREQ(rx_hdr[i], "bytes"))
+ rx_bytes_index = i + rx_offset;
+ else if (STREQ(rx_hdr[i], "packets"))
+ rx_packets_index = i + rx_offset;
+ else if (STREQ(rx_hdr[i], "errs"))
+ rx_errs_index = i + rx_offset;
+ else if (STREQ(rx_hdr[i], "drop"))
+ rx_drop_index = i + rx_offset;
+ }
+
+ for (i = 0; i < tx_nentries; i++) {
+ if (STREQ(tx_hdr[i], "bytes"))
+ tx_bytes_index = i + tx_offset;
+ else if (STREQ(tx_hdr[i], "packets"))
+ tx_packets_index = i + tx_offset;
+ else if (STREQ(tx_hdr[i], "errs"))
+ tx_errs_index = i + tx_offset;
+ else if (STREQ(tx_hdr[i], "drop"))
+ tx_drop_index = i + tx_offset;
+ }
Curious - what happens if STRNEQ in any of the above? That leaves the
various indices at 0 which is going to be rather bad I would think.
Perhaps you need a check that none of these *_index values are 0 (zero)
and force an error indicating something in the output has unexpectedly
changed.
+
+ while (fgets(line, sizeof(line), fp)) {
+ long long *stats_entries;
+
+ /* The stats line looks like:
+ * "eth0:..."
+ */
+
+ VIR_FREE(stats_row);
+ if (!(stats_row = virStringCleanExtraSpaces(line)))
+ goto cleanup;
+
+ virStringListFree(components_stats);
+ if (!(components_stats = virStringSplitCount(stats_row, " ", 0,
+ &ncomponents_stats)))
+ goto cleanup;
The ncomponent_stats had better be a certain size since component_stats
is deref'd below.
+
+ if (STREQLEN(components_stats[0], ifname, ifname_len)) {
Rather than another level of indent, you could use STRNEQLEN and continue;
I think also the logic could be (assuming the inlined cleaning of spaces:
virStringCleanExtraSpaces(line);
if (STRNEQLEN(line, ifname, ifname_len))
continue;
if (!(components_stats = virStringSplitCount(line, " ", 0,
&ncomponents_stats)))
goto cleanup;
+ if (VIR_ALLOC_N(stats_entries, rx_nentries +
tx_nentries) < 0)
+ goto cleanup;
+
+ for (i = 1; i < (rx_nentries + tx_nentries); i++) {
+ if (virStrToLong_ll(components_stats[i], NULL, 10,
+ (stats_entries + i -1)) < 0) {
s/ + i -1/ + i - 1/
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot parse %s stat position at
'%zu'"),
+ components_stats[i], i);
+ goto cleanup;
+ }
+ }> +
+ if (swapped) {
+ stats->rx_bytes = stats_entries[tx_bytes_index];
+ stats->rx_packets = stats_entries[tx_packets_index];
+ stats->rx_errs = stats_entries[tx_errs_index];
+ stats->rx_drop = stats_entries[tx_drop_index];
+ stats->tx_bytes = stats_entries[rx_bytes_index];
+ stats->tx_packets = stats_entries[rx_packets_index];
+ stats->tx_errs = stats_entries[rx_errs_index];
+ stats->tx_drop = stats_entries[rx_drop_index];
+ } else {
+ stats->rx_bytes = stats_entries[rx_bytes_index];
+ stats->rx_packets = stats_entries[rx_packets_index];
+ stats->rx_errs = stats_entries[rx_errs_index];
+ stats->rx_drop = stats_entries[rx_drop_index];
+ stats->tx_bytes = stats_entries[tx_bytes_index];
+ stats->tx_packets = stats_entries[tx_packets_index];
+ stats->tx_errs = stats_entries[tx_errs_index];
+ stats->tx_drop = stats_entries[tx_drop_index];
+ }
+ VIR_FREE(stats_entries);
+ ret = 0;
+ break;
I would goto cleanup; instead
+ }
+ }
And right here:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("/proc/net/dev: Interface statistics not found"));
IOW: A way to indicate that you've run off the end of the while loop.
You already added the interface %s not found above...
+ cleanup:
+ VIR_FORCE_FCLOSE(fp);
+ VIR_FREE(hdr1);
+ VIR_FREE(hdr2);
+ VIR_FREE(stats_row);
+ virStringListFree(components_hdr1);
+ virStringListFree(components_hdr2);
+ virStringListFree(components_stats);
+ virStringListFree(rx_hdr);
+ virStringListFree(tx_hdr);
+
+ return ret;
+}
+
#else /* !__linux__ */
int
virNetDevGetPhysPortID(const char *ifname ATTRIBUTE_UNUSED,
@@ -1622,7 +1814,15 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED,
return -1;
}
-
+int
+virNetDevGetProcNetdevStats(const char *ifname ATTRIBUTE_UNUSED,
+ virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED,
+ bool swapped ATTRIBUTE_UNUSED)
+{
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("interface stats not implemented on this platform"));
+ return -1;
+}
#endif /* !__linux__ */
#if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 71eaf45e30..4fb22562bb 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -266,6 +266,11 @@ int virNetDevGetVirtualFunctionInfo(const char *vfname, char
**pfname,
int *vf)
ATTRIBUTE_NONNULL(1);
+int virNetDevGetProcNetdevStats(const char *ifname,
+ virDomainInterfaceStatsPtr stats,
+ bool swapped)
The 2nd & 3rd args should align under "const char..."
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
Also because of 318d54e5 as I did with 425aac3a remove the
ATTRIBUTE_NONNULL(1)
John
+
int virNetDevGetFeatures(const char *ifname,
virBitmapPtr *out)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index bd0710ad2e..f0bce5ed34 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -687,76 +687,7 @@ virNetDevTapInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats,
bool swapped)
{
- int ifname_len;
- FILE *fp;
- char line[256], *colon;
-
- fp = fopen("/proc/net/dev", "r");
- if (!fp) {
- virReportSystemError(errno, "%s",
- _("Could not open /proc/net/dev"));
- return -1;
- }
-
- ifname_len = strlen(ifname);
-
- while (fgets(line, sizeof(line), fp)) {
- long long dummy;
- long long rx_bytes;
- long long rx_packets;
- long long rx_errs;
- long long rx_drop;
- long long tx_bytes;
- long long tx_packets;
- long long tx_errs;
- long long tx_drop;
-
- /* The line looks like:
- * " eth0:..."
- * Split it at the colon.
- */
- colon = strchr(line, ':');
- if (!colon) continue;
- *colon = '\0';
- if (colon-ifname_len >= line &&
- STREQ(colon-ifname_len, ifname)) {
- if (sscanf(colon+1,
- "%lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld
%lld %lld %lld %lld",
- &rx_bytes, &rx_packets, &rx_errs, &rx_drop,
- &dummy, &dummy, &dummy, &dummy,
- &tx_bytes, &tx_packets, &tx_errs, &tx_drop,
- &dummy, &dummy, &dummy, &dummy) != 16)
- continue;
-
- if (swapped) {
- stats->rx_bytes = tx_bytes;
- stats->rx_packets = tx_packets;
- stats->rx_errs = tx_errs;
- stats->rx_drop = tx_drop;
- stats->tx_bytes = rx_bytes;
- stats->tx_packets = rx_packets;
- stats->tx_errs = rx_errs;
- stats->tx_drop = rx_drop;
- } else {
- stats->rx_bytes = rx_bytes;
- stats->rx_packets = rx_packets;
- stats->rx_errs = rx_errs;
- stats->rx_drop = rx_drop;
- stats->tx_bytes = tx_bytes;
- stats->tx_packets = tx_packets;
- stats->tx_errs = tx_errs;
- stats->tx_drop = tx_drop;
- }
-
- VIR_FORCE_FCLOSE(fp);
- return 0;
- }
- }
- VIR_FORCE_FCLOSE(fp);
-
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("/proc/net/dev: Interface not found"));
- return -1;
+ return virNetDevGetProcNetdevStats(ifname, stats, swapped);
}
#elif defined(HAVE_GETIFADDRS) && defined(AF_LINK)
int