On 06/28/2018 09:22 AM, Jai Singh Rana wrote:

This patch adds string manupulation helper function which takes

manipulation

string as input and returns string with all but one space removed between letters, numbers or words.

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@cavium.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ 3 files changed, 40 insertions(+)

Sorry for the delay on reviewing this series. I was hoping that someone more familiar with details of network/vf stats would jump in...

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..272e7426dd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2891,6 +2891,7 @@ virSkipSpacesBackwards; virStrcpy; virStrdup; virStringBufferIsPrintable; +virStringCleanExtraSpaces; virStringEncodeBase64; virStringFilterChars; virStringHasChars; diff --git a/src/util/virstring.c b/src/util/virstring.c index 15f367af7c..1f45b2b553 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -139,6 +139,42 @@ virStringSplit(const char *string,

/** + * virCleanExtraSpacesInString:

This ^^^ doesn't match...

+ * @src: original null terminated string + * + * Returns string with all spaces but one removed between words in @src + * string. Caller is responsible for freeing the returned string. + * Returns NULL if new string could not be allocated. + * + */ +char * +virStringCleanExtraSpaces(char *src)

... this ^^^ name. Consider how virStringStripControlChars handles stripping out control characters and then modify to skip "if (c_isspace())" perhaps using virSkipSpaces.. Since you don't really care if the line is edited "in place", consider just moving characters, then you don't have to VIR_ALLOC_N something that you're not trimming at the end anyway. You should add a test to tests/virstringtest.c similar to how other existing character stripping tests do it, but of course proving that you're stripping spaces as you expect. Don't forget to alter the commit message to note that you're editing the passed string "in place" and change the function to void. John

+{ + char *dst; + size_t dstlen; + int src_at = 0; + int dst_at; + + dstlen = strlen(src); + if (VIR_ALLOC_N(dst, dstlen) < 0) + return NULL; + + while (src[src_at] == ' ') + src_at++; + + for (dst_at = 0; src[src_at] != '\0'; src_at++) { + if (src[src_at + 1] == ' ' && src[src_at] == ' ') + continue; + dst[dst_at] = src[src_at]; + dst_at++; + } + dst[dst_at] = '\0'; + + return dst; +} + + +/** * virStringListJoin: * @strings: a NULL-terminated array of strings to join * @delim: a string to insert between each of the strings diff --git a/src/util/virstring.h b/src/util/virstring.h index 607ae66e99..0778bc45c8 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -37,6 +37,9 @@ char **virStringSplit(const char *string, size_t max_tokens) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+char *virStringCleanExtraSpaces(char *src) + ATTRIBUTE_NONNULL(1); + char *virStringListJoin(const char **strings, const char *delim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

$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@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

On 06/28/2018 09:22 AM, Jai Singh Rana wrote:

Introduce new APIs virNetdevHostdevGetVFRIfName and virNetdevHostdevCheckVFRIfName to fetch/verify VF Representor name according to switch model for network devices in linux.

Switchdev VF representor interface name on host is queried based on Bus:Device:Function information of pci SR-IOV device in Domain's hostdev def and subsequently verifying the required net sysfs directory and file entries of VF representor.

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@cavium.com> --- po/POTFILES | 1 + src/libvirt_private.syms | 7 ++ src/util/Makefile.inc.am | 2 + src/util/virhostdev.c | 2 +- src/util/virhostdev.h | 8 ++ src/util/virnetdevhostdev.c | 300 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevhostdev.h | 34 +++++ 7 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 src/util/virnetdevhostdev.c create mode 100644 src/util/virnetdevhostdev.h

This will certainly be where things get a bit more "unknown" to me... Still I think this patch can be split in half - with one patch being just the promotion of virHostdevNetDevice to external and the second adding the new module.

diff --git a/po/POTFILES b/po/POTFILES index be2874487c..3f8731f342 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -235,6 +235,7 @@ src/util/virmdev.c src/util/virnetdev.c src/util/virnetdevbandwidth.c src/util/virnetdevbridge.c +src/util/virnetdevhostdev.c src/util/virnetdevip.c src/util/virnetdevmacvlan.c src/util/virnetdevmidonet.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ba8cd2a14..5aa8f7ed64 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1968,6 +1968,7 @@ virHostdevFindUSBDevice; virHostdevIsMdevDevice; virHostdevIsSCSIDevice; virHostdevManagerGetDefault; +virHostdevNetDevice; virHostdevPCINodeDeviceDetach; virHostdevPCINodeDeviceReAttach; virHostdevPCINodeDeviceReset; @@ -2356,6 +2357,12 @@ virNetDevBridgeSetSTPDelay; virNetDevBridgeSetVlanFiltering;

+# util/virnetdevhostdev.h +virNetdevHostdevCheckVFRIfName; +virNetdevHostdevGetVFRIfName; +virNetdevHostdevVFRIfStats; + + # util/virnetdevip.h virNetDevIPAddrAdd; virNetDevIPAddrDel; diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index a22265606c..0846a8e025 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -108,6 +108,8 @@ UTIL_SOURCES = \ util/virnetdevbandwidth.h \ util/virnetdevbridge.c \ util/virnetdevbridge.h \ + util/virnetdevhostdev.c \ + util/virnetdevhostdev.h \ util/virnetdevip.c \ util/virnetdevip.h \ util/virnetdevmacvlan.c \ diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f4bd19df64..c6ee725860 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -303,7 +303,7 @@ virHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev) }

Could you add a brief header here describing the inputs and what it returns. Perhaps of most interest is the values that @pfNetDevIdx can be (I see -1, 0, 1 being used). It also looks like @linkdev and @vf are returned as well as 0, -1 for success/failure. The @vf can be -1 and ret == 0 if the else condition is followed. Useful things to know for external consumers.

-static int +int virHostdevNetDevice(virDomainHostdevDefPtr hostdev, int pfNetDevIdx, char **linkdev, diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 8f77c00221..7412a20aa9 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -60,6 +60,14 @@ struct _virHostdevManager { };

virHostdevManagerPtr virHostdevManagerGetDefault(void); + +int +virHostdevNetDevice(virDomainHostdevDefPtr hostdev, + int pfNetDevIdx, + char **linkdev, + int *vf) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);

(3) had better be NONNULL too!

+ int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, diff --git a/src/util/virnetdevhostdev.c b/src/util/virnetdevhostdev.c new file mode 100644 index 0000000000..2d3e46c81f --- /dev/null +++ b/src/util/virnetdevhostdev.c @@ -0,0 +1,300 @@ +/* + * virnetdevhostdev.c: utilities to get/verify Switchdev VF Representor

Far be it for me to be the license expert, but there's no copyright year here. I would have at least expected something, such as: * Copyright (C) 2018 XXX where for XXX I've seen people add a company name or their own name - there's lots of examples to choose from. Not my exact area of expertise over the "right value" though. I would put Red Hat Inc. there, but then again, that's where I work.

+ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <dirent.h> + +#include "virhostdev.h" +#include "virnetdev.h" +#include "virnetdevhostdev.h" +#include "viralloc.h" +#include "virstring.h" +#include "virfile.h" +#include "virerror.h" +#include "virlog.h" +#include "c-ctype.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.netdevhostdev"); + +#ifndef IFNAMSIZ +# define IFNAMSIZ 16 +#endif + +#define IFSWITCHIDSIZ 20 + +#ifdef __linux__ +/** + * virNetdevHostdevNetSysfsPath + * + * @pf_name: netdev name of the physical function (PF) + * @vf: virtual function (VF) number for the device of interest + * @vf_ifname: name of the VF representor interface + * + * Finds the VF representor name of VF# @vf of SRIOV PF @pfname, + * and puts it in @vf_ifname. The caller must free @vf_ifname + * when it's finished with it + * + * Returns 1 on success, 0 for no switchdev support for @pfname device

Typically we prefer -1 on failure, 0 on success Still you seem to be having a 3rd condition where 0 could be we failed and we don't care and 1 could be we succeeded. The -1 could be left as a fatal error since currently there'd be no way to determine the difference between OOM from virAsprintf or failure from virFileReadAllQuiet. In both cases the caller should ignore/reset the last error rather than this lower down API.

+ */ +static int +virNetdevHostdevNetSysfsPath(char *pf_name, + int vf, + char **vf_ifname) +{ + size_t i; + char *pf_switch_id = NULL; + char *pf_switch_id_file = NULL; + char *pf_subsystem_device_file = NULL; + char *pf_subsystem_device_switch_id = NULL; + char *pf_subsystem_device_port_name_file = NULL; + char *pf_subsystem_dir = NULL; + char *vf_rep_ifname = NULL; + int vf_rep_num; + const char *vf_num_str; + DIR *dirp = NULL; + struct dirent *dp; + int ret = 0; + + if (virAsprintf(&pf_switch_id_file, SYSFS_NET_DIR "%s/phys_switch_id", + pf_name) < 0) + goto cleanup;

should be a fatal failure

+ + if (!virFileExists(pf_switch_id_file)) + goto cleanup;

could be a graceful failure

+ + /* If file exists, failure to read or if it's an empty file just means + * that driver doesn't support phys_switch_id, therefore ignoring the error

"that the driver" s/ignoring/ignore

+ * from virFileReadAllQuiet(). + */ + if (virFileReadAllQuiet(pf_switch_id_file, IFSWITCHIDSIZ, + &pf_switch_id) <= 0) + goto cleanup; +

Seems like this could be a graceful failure

+ if (virAsprintf(&pf_subsystem_dir, SYSFS_NET_DIR "%s/subsystem", + pf_name) < 0) + goto cleanup;

Would be a fatal failure. I'll let you decide on the rest.

+ + if (virDirOpen(&dirp, pf_subsystem_dir) < 0) + goto cleanup; + + /* Iterate over the PFs subsystem devices to find entry with matching + * switch_id with that of PF. + */ + while (virDirRead(dirp, &dp, pf_subsystem_dir) > 0) { + if (STREQ(dp->d_name, pf_name)) + continue; + + VIR_FREE(pf_subsystem_device_file); + if (virAsprintf(&pf_subsystem_device_file, "%s/%s/phys_switch_id", + pf_subsystem_dir, dp->d_name) < 0) + goto cleanup; + + if (!virFileExists(pf_subsystem_device_file)) + goto cleanup; + + /* If file exists, failure to read or if it's an empty file just means + * the driver doesn't support the entry being probed for current + * device in subsystem dir, therefore ignoring the error in the + * following calls to virFileReadAllQuiet() and continue the loop + * to find device which supports this and is a match. + */ + VIR_FREE(pf_subsystem_device_switch_id); + if (virFileReadAllQuiet(pf_subsystem_device_file, IFSWITCHIDSIZ, + &pf_subsystem_device_switch_id) > 0) { + if (STRNEQ(pf_switch_id, pf_subsystem_device_switch_id)) + continue; + } + else + continue; + + VIR_FREE(pf_subsystem_device_port_name_file); + if (virAsprintf(&pf_subsystem_device_port_name_file, + "%s/%s/phys_port_name", pf_subsystem_dir, + dp->d_name) < 0) + goto cleanup; + + if (!virFileExists(pf_subsystem_device_port_name_file)) + goto cleanup; + + VIR_FREE(vf_rep_ifname); + if (virFileReadAllQuiet + (pf_subsystem_device_port_name_file, IFNAMSIZ, + &vf_rep_ifname) <= 0) + continue; + + /* phys_port_name may contain just VF number or string in format + * as pf'X'vf'Y' or vf'Y', where X and Y are PF and VF numbers. + * As at this point, we are already with correct PF, just need + * to verify VF number which is always at the end. + */ + + /* vf_rep_ifname read from file may contain new line,replace with '\0' + for string comparison below */ + i = strlen(vf_rep_ifname); + if (c_isspace(vf_rep_ifname[i-1])) { + vf_rep_ifname[i-1] = '\0'; + i -= 1; + } + + /* Locating the start of numeric portion of VF in the string */ + while (c_isdigit(vf_rep_ifname[i-1])) + i -= 1; + + vf_num_str = vf_rep_ifname + i; + vf_rep_num = virParseNumber(&vf_num_str); + + if (vf_rep_num == vf) { + if (VIR_STRDUP(*vf_ifname, dp->d_name) < 0) + goto cleanup; + ret = 1; + break;

Could be a goto cleanup too... because...

+ } + }

...if we complete the virDirRead loop without any match is that a bad thing?

+ + cleanup: + VIR_DIR_CLOSE(dirp); + VIR_FREE(pf_switch_id); + VIR_FREE(pf_switch_id_file); + VIR_FREE(pf_subsystem_dir); + VIR_FREE(pf_subsystem_device_file); + VIR_FREE(pf_subsystem_device_switch_id); + VIR_FREE(pf_subsystem_device_port_name_file); + VIR_FREE(vf_rep_ifname); + return ret; +} + + +/** + * virNetdevHostdevGetVFRepIFName

^^^ This ....

+ * + * @hostdev: host device to check + * + * Returns VF string with VF representor name upon success else NULL + */ +char * +virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev)

... doesn't match this ^^^^ Asking from the point of not sure, but is "VFRIfName" redundant? That is could it just be "VFRName" or is there a delineation between a VFR Name and a VFR IFName that needs to be made. You may want to just call this "GetVFRInfo" leaving for the possibility of future expansion to more than just @ifname. Thus you pass the @ifname as a parameter and use 0 and -1 return values to indicate success or failure.

+{ + char *linkdev = NULL; + char *ifname = NULL; + char *vf_ifname = NULL; + int vf = -1; + + if (virHostdevNetDevice(hostdev, -1, &linkdev, &vf) < 0) + goto cleanup; + + if (!virNetdevHostdevNetSysfsPath(linkdev, vf, &vf_ifname)) { + virResetLastError();

This is what I meant earlier - you have no way to determine if the failure is fatal or not and resetting an OOM may not acceptible from this caller. I think the caller of this API should make that determination (something I validate later on).

+ goto cleanup; + } + + ignore_value(VIR_STRDUP(ifname, vf_ifname));

Why not just return vf_ifname directly later? It's already a VIR_STRDUP'd value - especially since you VIR_FREE it below.

+ + cleanup: + VIR_FREE(linkdev); + VIR_FREE(vf_ifname); + return ifname; +} + + +/** + * virNetdevHostdevCheckVFRepIFName + * + * @hostdev: host device to check + * @ifname : VF representor name to verify + * + * Returns true on success, false on failure + */ +bool +virNetdevHostdevCheckVFRIfName(virDomainHostdevDefPtr hostdev, + const char *ifname)

Same notes about the method name

+{ + char *linkdev = NULL; + char *vf_ifname = NULL; + int vf = -1; + bool ret = false; + + if (virHostdevNetDevice(hostdev, -1, &linkdev, &vf) < 0) + goto cleanup; + + if (!virNetdevHostdevNetSysfsPath(linkdev, vf, &vf_ifname)) { + virResetLastError();

Again, you really don't know the reason for failure here. This is a Check method with a couple of ways to fail. Even the previous call could have some sort of failure that isn't reset. So prehaps the Reset should be in cleanup in this case.

+ goto cleanup; + } + + if (STREQ(ifname, vf_ifname)) + ret = true;

ret = STREQ(ifname, vf_ifname);

+ + cleanup: + VIR_FREE(linkdev); + VIR_FREE(vf_ifname); + return ret; +} + + +/** + * virNetdevHostdevVFRepInterfaceStats: + * @ifname: interface + * @stats: where to store statistics + * @swapped: whether to swap RX/TX fields + * + * Returns 0 on success, -1 otherwise (with error reported). + */ +int +virNetdevHostdevVFRIfStats(const char *ifname, + virDomainInterfaceStatsPtr stats, + bool swapped) +{ + return virNetDevGetProcNetdevStats(ifname, stats, swapped);

Not sure I ever see the need for this to be specific... You're not validating that ifname is a VFRIfName, so the GetProcNetdevStats could just be called directly instead when it's needed later on. If by chance there could be multiple ways to obtain stats for one of these I could see use for the wrapper, but that doesn't happen in this patch series, so the direct call is better since the assumption is that @ifname is a vf_ifname (from the eventual caller).

+} +#else +static const char *unsupported = N_("not supported on non-linux platforms"); + +

If any of the above in the #ifdef __linux__ do change, don't forget to modify these as well!

+char * +virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return NULL; +} + + +bool +virNetdevHostdevCheckVFRIfName(virDomainHostdevDefPtr hostdev ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return false; +} + + +int +virNetdevHostdevVFRIfStats(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"));

Why the change here? IDC, but consisitently speaking it's different.

+ return -1; +} +#endif diff --git a/src/util/virnetdevhostdev.h b/src/util/virnetdevhostdev.h new file mode 100644 index 0000000000..5fb846fad5 --- /dev/null +++ b/src/util/virnetdevhostdev.h @@ -0,0 +1,34 @@ +/* + * virnetdevhostdev.h: utilities to get/verify Switchdev VF Representor + *

Same here w/ copyright year

+ * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __VIR_NETDEV_HOSTDEV_H__ +# define __VIR_NETDEV_HOSTDEV_H__ + +char * virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

You can follow/copy the .c header here and the other two: char * virNetdevHostdevGetVFRIfName(...) John

+ +bool virNetdevHostdevCheckVFRIfName(virDomainHostdevDefPtr hostdev, + const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virNetdevHostdevVFRIfStats(const char *ifname, + virDomainInterfaceStatsPtr stats, + bool swapped) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +#endif /* __VIR_NETDEV_HOSTDEV_H__ */

$SUBJ: "conf: Introduce virDomainNetFindByHostdev" Is fine. then the commit message can be more descriptive. On 06/28/2018 09:22 AM, Jai Singh Rana wrote:

Introduce API virDomainNetFindByHostdev which retrieves net def in given domain for the given hostdev def. This API will now be used by virDomainNetFind to further probe net for hostdev network devices.

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@cavium.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 2 ++ src/util/virhostdev.c | 2 +- src/util/virhostdev.h | 3 +++ 5 files changed, 51 insertions(+), 1 deletion(-)

You could split out the last 3 into their own patch to promote virHostdevIsPCINetDevice from static to global, but that's just a nit.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8cb7f37f3..8432215d19 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -56,6 +56,7 @@ #include "virsecret.h" #include "virstring.h" #include "virnetdev.h" +#include "virnetdevhostdev.h" #include "virnetdevmacvlan.h" #include "virhostdev.h" #include "virmdev.h" @@ -29064,6 +29065,37 @@ virDomainNetFind(virDomainDefPtr def, const char *device)

/** + * virDomainNetFindByHostdev: + * @def: domain's def + * @hostdev: hostdev whose net def if exists to be retrieved + * + * Finds net def in domain given the domain's hostdev. + * + * Returns a pointer to the net def or NULL if not found. + */ +virDomainNetDefPtr +virDomainNetFindByHostdev(virDomainDefPtr def, + virDomainHostdevDefPtr hostdev) +{ + size_t i; + virDomainNetType actualType; + virDomainHostdevDefPtr hostdef = NULL; + + for (i = 0; i < def->nnets; i++) { + actualType = virDomainNetGetActualType(def->nets[i]); + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) + hostdef = virDomainNetGetActualHostdev(def->nets[i]);

Of concern, virDomainNetGetActualHostdev can return NULL...

+ else + continue; + if (!memcmp(hostdev, hostdef, sizeof(virDomainHostdevDef)))

Is comparing the whole thing necessary? Is perhaps just the name good enough? There's a lot of data in the HostdevDef - if something doesn't match then I suppose you could conceivably fail here even though you're not expecting to.

+ return def->nets[i]; + }

Does the following work? for (i = 0; i < def->nnets; i++) { virDomainHostdevDefPtr net_hostdev; if (!(net_hostdev = virDomainNetGetActualHostdev(def->nets[i]))) continue; [whatever comparison is appropriate for whether net_hostdev matches the passed hostdev]

+ + return NULL; +} + + +/** * virDomainNetFindByName: * @def: domain's def * @ifname: interface name @@ -29077,12 +29109,23 @@ virDomainNetFindByName(virDomainDefPtr def, const char *ifname) { size_t i; + virDomainNetDefPtr net = NULL;

for (i = 0; i < def->nnets; i++) { if (STREQ_NULLABLE(ifname, def->nets[i]->ifname)) return def->nets[i]; }

+ /* Give a try to hostdev if its a switchdev network device*/

s/its/it's/ s/device*/device */

+ for (i = 0; i < def->nhostdevs; i++) { + if (!virHostdevIsPCINetDevice(def->hostdevs[i])) + continue; + if (virNetdevHostdevCheckVFRIfName(def->hostdevs[i], ifname)) { + if ((net = virDomainNetFindByHostdev(def, def->hostdevs[i]))) + return net; + } + } +

IIUC, this is the "magic" place where as stated in the doc patch6 where the hostdev being used to fetch the stats is determined to be an SR-IOV device w/ VF representor on host in switchdev mode. There is some concern over adding this without some sort of "filter" - mainly because of the extra work, but also because it's not 100% clear to me all callers would expect to get this data. Anyway, for callers of virDomainNetFind that end up just calling virNetDevTapInterfaceStats - it would seem to be OK, but I'm not 100% sure. For callers that are getting/setting interface parameters - would returning the @net that is actually a hostdev type be expected? Maybe it'd be best to introduce a boolean 'wantSRIOVHostdevVFR' that would be "true" only from qemuDomainInterfaceStats since it then has the logic to detect if the ActualType(net) is HOSTDEV (name chosen from my IIUC above - feel free to adjust to match reality). That way we only check the hostdev's if that's what we want with the logic being: if (!wantSRIOVHostdevVFR) return NULL; for (i = 0; i < def->nhostdevs; i++) { ... John

return NULL; }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0924fc4f3c..ccec74e51d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3146,6 +3146,8 @@ int virDomainDiskSourceParse(xmlNodePtr node,

int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net); virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device); +virDomainNetDefPtr virDomainNetFindByHostdev(virDomainDefPtr def, + virDomainHostdevDefPtr hostdev); virDomainNetDefPtr virDomainNetFindByName(virDomainDefPtr def, const char *ifname); bool virDomainHasNet(virDomainDefPtr def, virDomainNetDefPtr net); int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5aa8f7ed64..e4d8583d41 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -449,6 +449,7 @@ virDomainNetDefClear; virDomainNetDefFormat; virDomainNetDefFree; virDomainNetFind; +virDomainNetFindByHostdev; virDomainNetFindByName; virDomainNetFindIdx; virDomainNetGenerateMAC; @@ -1966,6 +1967,7 @@ virHostCPUStatsAssign; # util/virhostdev.h virHostdevFindUSBDevice; virHostdevIsMdevDevice; +virHostdevIsPCINetDevice; virHostdevIsSCSIDevice; virHostdevManagerGetDefault; virHostdevNetDevice; diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index c6ee725860..2b8ecb9649 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -348,7 +348,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, }

-static bool +bool virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev) { return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 7412a20aa9..71faaf4e7a 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -197,6 +197,9 @@ virHostdevReAttachDomainDevices(virHostdevManagerPtr mgr, const char *oldStateDir) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); bool +virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev) + ATTRIBUTE_NONNULL(1); +bool virHostdevIsSCSIDevice(virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1); bool

On 06/28/2018 09:22 AM, Jai Singh Rana wrote:

In case of pci SR-IOV device with interface_type as 'hostdev', return network stats if it has a VF Representor interface enabled on host for pci SR-IOV device according to switchdev model.

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@cavium.com> --- src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e94b4f095..167807704b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -67,6 +67,7 @@ #include "virbuffer.h" #include "virhostcpu.h" #include "virhostmem.h" +#include "virnetdevhostdev.h" #include "virnetdevtap.h" #include "virnetdevopenvswitch.h" #include "capabilities.h" @@ -11258,6 +11259,10 @@ qemuDomainInterfaceStats(virDomainPtr dom, if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { if (virNetDevOpenvswitchInterfaceStats(net->ifname, stats) < 0) goto cleanup; + } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + bool swapped = virDomainNetTypeSharesHostView(net); + if (virNetdevHostdevVFRIfStats(device, stats, !swapped) < 0) + goto cleanup; Based on feedback from 4/6, this is the patch where the virDomainNetFind can then use the True argument since the HOSTDEV part is handled in order to do the determination for hostdev using SR-IOV w/ VF Representor in switchdev mode.

} else { if (virNetDevTapInterfaceStats(net->ifname, stats, !virDomainNetTypeSharesHostView(net)) < 0) @@ -19935,6 +19940,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, { size_t i; struct _virDomainInterfaceStats tmp; + char *vf_ifname = NULL; int ret = -1;

if (!virDomainObjIsActive(dom)) @@ -19947,21 +19953,41 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, virDomainNetDefPtr net = dom->def->nets[i]; virDomainNetType actualType;

- if (!net->ifname) + actualType = virDomainNetGetActualType(net); + + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + vf_ifname = virNetdevHostdevGetVFRIfName(dom->def->hostdevs[i]);

This is where I'd point out that a fatal error from *GetVFRIfName probably isn't a good thing to continue from. Still I see other code in there that resets last error and continues, so perhaps *this* is where that happens rather than in patch3... That is let/force the caller decide what it wants to ignore rather than having the lower in the stack method handle that logic.

+ if (!vf_ifname) + continue; + } + else if (!net->ifname) continue;

memset(&tmp, 0, sizeof(tmp));

- actualType = virDomainNetGetActualType(net);

- QEMU_ADD_NAME_PARAM(record, maxparams, - "net", "name", i, net->ifname); + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", "name", i, vf_ifname); + else + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", "name", i, net->ifname);

if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) { virResetLastError(); continue; } + } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + int rc; + bool swapped = virDomainNetTypeSharesHostView(net); + + rc = virNetdevHostdevVFRIfStats(vf_ifname, &tmp, !swapped); + VIR_FREE(vf_ifname); + if (rc < 0) { + virResetLastError();

hmm.. well this shows that this caller doesn't care, so maybe fatal failures aren't so bad. Although since virNetdevHostdevVFRIfStats is just a wrapper to virNetDevGetProcNetdevStats, then as I pointed out earlier the direct call to virNetdevHostdevNetSysfsPath is fine. John

+ continue; + } } else { if (virNetDevTapInterfaceStats(net->ifname, &tmp, !virDomainNetTypeSharesHostView(net)) < 0) {

On 06/28/2018 09:22 AM, Jai Singh Rana wrote:

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@cavium.com> --- docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+)

This will need to be adjusted for top of tree.

diff --git a/docs/news.xml b/docs/news.xml index 468d34093a..c6394a8f0f 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -130,6 +130,15 @@ or virStorageVolCreateXMLFrom. </description> </change> + <change> + <summary> + qemu: Support interface network stats for VF Representors + </summary> + <description> + Interface network stats are supported now for SR-IOV device(hostdev)

s/device(hostdev/device (hostdev/

+ if this interface has VF representor on host in switchdev mode.

John

+ </description> + </change> </section> <section title="Bug fixes"> <change>