On 04/04/2018 12:29 PM, Jai Singh Rana wrote: > Switchdev VF representor interface name on host is queried based on > Bus:Device:Function information of pci SR-IOV device in Domain's > 'hostdev' structure and subsequently verifying the required net sysfs > directory and file entries of VF representor according to switchdev > model. You are missing the S-o-b: The someone new policy is that: Contributors to libvirt projects must assert that they are in compliance with the Developer Certificate of Origin 1.1. This is achieved by adding a "Signed-off-by" line containing the contributor's name and e-mail to every commit message. The presence of this line attests that the contributor has read the above lined DCO and agrees with its statements. https://developercertificate.org/ > --- > v3 includes changes based on v2's[1] feedback and suggestions. Fixes > warnings reported by syntax-check. > [1] https://www.redhat.com/archives/libvir-list/2018-February/msg00562.html > > po/POTFILES.in | 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 | 374 ++++++++++++++++++++++++++++++++++++++++++++ > src/util/virnetdevhostdev.h | 35 +++++ > 7 files changed, 428 insertions(+), 1 deletion(-) > create mode 100644 src/util/virnetdevhostdev.c > create mode 100644 src/util/virnetdevhostdev.h > You probably don't have cppi installed, but if you did it would have pointed out a few more syntax-check issues... > diff --git a/po/POTFILES.in b/po/POTFILES.in > index d84859a4e..8cd6b86e8 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -234,6 +234,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 f6897915c..fad235206 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1923,6 +1923,7 @@ virHostCPUStatsAssign; > virHostdevFindUSBDevice; > virHostdevIsSCSIDevice; > virHostdevManagerGetDefault; > +virHostdevNetDevice; > virHostdevPCINodeDeviceDetach; > virHostdevPCINodeDeviceReAttach; > virHostdevPCINodeDeviceReset; > @@ -2306,6 +2307,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 a3c3b711f..31fe11c68 100644 > --- a/src/util/Makefile.inc.am > +++ b/src/util/Makefile.inc.am > @@ -104,6 +104,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 a12224c58..4f7b46a04 100644 > --- a/src/util/virhostdev.c > +++ b/src/util/virhostdev.c > @@ -306,7 +306,7 @@ virHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev) > } > > > -static int > +int > virHostdevNetDevice(virDomainHostdevDefPtr hostdev, > int pfNetDevIdx, > char **linkdev, > diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h > index 54e1c66be..735220add 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); > + > 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 000000000..19f95bfdd > --- /dev/null > +++ b/src/util/virnetdevhostdev.c > @@ -0,0 +1,374 @@ > +/* > + * virnetdevhostdev.c: utilities to get/verify Switchdev VF Representor > + * > + * 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 cppi: src/util/virnetdevhostdev.c: line 41: not properly indented IOW: "# define" > +#endif > + > +#define IFSWITCHIDSIZ 20 > + > +#ifdef __linux__ FWIW: Easiest way to test that your compilation works on "linux" and something else is to simply change this line to something else that wouldn't be defined and that would show if your #else logic works... > +/** > + * 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 0 on success, -1 on failure > + */ > +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; > + char *vf_num_str = NULL; > + DIR *dirp = NULL; > + struct dirent *dp; > + int ret = -1; > + > + if (virAsprintf(&pf_switch_id_file, SYSFS_NET_DIR "%s/phys_switch_id", > + pf_name) < 0) > + goto cleanup; > + > + if (virAsprintf(&pf_subsystem_dir, SYSFS_NET_DIR "%s/subsystem", > + pf_name) < 0) > + goto cleanup; > + > + /* a failure to read just means the driver doesn't support > + * phys_switch_id, so ignoring the error from > + * virFileReadAllQuiet(). > + */ > + if (virFileReadAllQuiet(pf_switch_id_file, IFSWITCHIDSIZ, > + &pf_switch_id) <= 0) { A < 0 is an error A == 0 is empty file so be careful in just cut-copy-paste as this will fail for some unknown reason if the length of what's read == 0. This should also go after the virAsprintf for pf_switch_id_file > + goto cleanup; > + } And you don't really the { } for a one line goto cleanup although it passed syntax-check because of the multi-line virFileReadAddQuiet Your comments though make it seem like you want to check if the file exists first before reading (e.g. if (!virFileExists())). Then if it doesn't exist, return. You may also want to alter your returns to be -1 error, 0 nothing gathered, and 1 something gathered. Don't forget to document that too. Looking at patch 2 which is where virNetdevHostdevGetVFRIfName (the caller to this method) is called - if the returned name is NULL, then the data would be ignored anyway. So the 0 and 1 return could be useful. > + > + 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; > + > + /* a failure to read just means the driver doesn't support the entry > + * being probed for current device in subsystem dir, so ignoring the > + * error in the following calls to virFileReadAllQuiet() and continue > + * the loop to find device which supports this and is a match. s/a match/a match/ > + */ Also, entire comment is incorrectly indented. Again, should we first to a if (!virFileExists()) continue; type operation? > + 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; So if <= we're falling through to the next step, which I don't think you want. > + } > + This one doesn't follow your pattern to add a VIR_FREE(pf_subsystem_device_port_name_file) beforehand within your while loop... > + if (virAsprintf(&pf_subsystem_device_port_name_file, > + "%s/%s/phys_port_name", pf_subsystem_dir, > + dp->d_name) < 0) > + goto cleanup; > + > + VIR_FREE(vf_rep_ifname); > + vf_rep_ifname = NULL; Should be unnecessary since virFree will do that for you Again more thought around !virFileExists() > + > + if (virFileReadAllQuiet > + (pf_subsystem_device_port_name_file, IFNAMSIZ, > + &vf_rep_ifname) <= 0) > + continue; > + Again the pattern of VIR_FREE(vf_num_str); beforehand > + if (virAsprintf(&vf_num_str, "%d", vf) < 0) > + goto cleanup; > + > + /* 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 now. > + */ > + > + /* 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; > + } > + > + while (c_isdigit(vf_rep_ifname[i-1])) > + i -= 1; > + > + if ((ret = STREQ((vf_rep_ifname + i), vf_num_str))) { This changes ret < == > 0... don't believe that's what you really want. > + if (VIR_STRDUP(*vf_ifname, dp->d_name) < 0) > + goto cleanup; > + ret = 0; > + break; > + } For the above last hunk... All if this because you're trying to determine if the passed "vf" number is equal to the number portion of the vf_rep_ifname? I believe virSkipSpaces and virParseNumber will help you be a whole lot cleaner! > + } > + > + 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_num_str); > + VIR_FREE(vf_rep_ifname); > + return ret; > +} > + > + > +/** > + * virNetdevHostdevGetVFRepIFName > + * > + * @hostdev: host device to check > + * > + * Returns VF string with VF representor name upon success else NULL > + */ > +char * > +virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev) > +{ > + 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))> + goto cleanup; Considering my comment above and changing the return values means this could change as well. At the very least actually checking < 0 for failure... To some degree since the caller only cares if the return is NULL or not and doesn't error when it's not, then all/any error generated will be lost. So that means would could be a valid reason to cause a failure would be lost and the data just ignored. At the very least a virResetLastError would clear away the error > + > + ignore_value(VIR_STRDUP(ifname, vf_ifname)); > + > + 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) > +{ > + 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)) > + goto cleanup; > + > + if (STREQ(ifname, vf_ifname)) > + ret = true; > + > + cleanup: > + VIR_FREE(linkdev); > + VIR_FREE(vf_ifname); > + return ret; > +} > + > + > +/*-------------------- interface stats --------------------*/ > +/* Copy of virNetDevTapInterfaceStats for linux */ Before I read this comment after looking at the next patch and wondering what was different to virNetDevTapInterfaceStats, my first gut response follows. Now that I've dealt with that - rather than cut-n-paste-n-copy - you need to figure out a way to make use of the existing code. That means introducing an API that the __linux___ version of virNetDevTapInterfaceStats would call and this function would also call. Perhaps something along the lines of virNetDevGetProcNetDev that lives in src/util/virnetdev.c... One patch would extract it out and the next one would also use it for your hostdev path. FWIW: Still some of my feeings below could still apply to/for an improved processing of the file/data - especially w/r/t format of the file, headings, etc. Since "something" already exists though, we can live with it I suppose, even though it's perhaps not optimal > +/** > + * virNetdevHostdevVFRepInterfaceStats: > + * @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 > +virNetdevHostdevVFRIfStats(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. > + */ Tehnically there's a header row or two that you're missing from what I see... > + colon = strchr(line, ':'); Once you get beyond and verify the header, you may want to look into virStringSplit - it's pretty powerful and this is just ripe for all sorts of issues especially if the format of the output *ever* changes.... In fact I'd go as far to say that checking/learning which column heading goes with which data element might be very useful thing to add here especially if it ever differs between distros or ever has changed or could change. Check out using virFileReadAll and other API's that read /proc files. If you can "map" your expected headers into the format you're reading and ensure that no future change would alter what you've magically sscanf'd below, then that future proofs yourself a bit. You could possibly also create some magic that would pull only the ones you care about based on the headers provided. There's "inventive ways" to use ARRAY_CARDINALITY (see virNetClientFindDefaultSshKey) that will help you make sure that the header doesn't change which skews your results. In any case, the folowing at the very least should follow virHostCPUGetStatsLinux or virHostMemGetStatsLinux in order to make sure headers match sscanf'd columns. > + 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")); Wouldn't this be more an empty /proc/net/dev file? Is that a sign of a different problem? We've already covered the missing file above. > + return -1; > +} > +#else > +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")); > + return -1; > +} > + > + > +static const char *unsupported = N_("not supported on non-linux platforms"); > + > + > +static int static functions don't need to be in the #else > +virNetdevHostdevNetSysfsPath(char *pf_name ATTRIBUTE_UNUSED, > + int vf ATTRIBUTE_UNUSED, > + char **vf_ifname ATTRIBUTE_UNUSED) > +{ > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); > + return -1; > +} > + > + > +char * > +virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev ATTRIBUTE_UNUSED, > + char **ifname ATTRIBUTE_UNUSED) Parameters here are different than .h file. > +{ > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); > + return NULL; > +} > + > + > +static bool This one's not static... > +virNetdevHostdevCheckVFRIfName(virDomainHostdevDefPtr hostdev ATTRIBUTE_UNUSED, > + const char *ifname ATTRIBUTE_UNUSED) > +{ > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); > + return false; > +} Please place the order of functions in the #else section the same as in the #if section > +#endif > diff --git a/src/util/virnetdevhostdev.h b/src/util/virnetdevhostdev.h > new file mode 100644 > index 000000000..3ea1804ff > --- /dev/null > +++ b/src/util/virnetdevhostdev.h > @@ -0,0 +1,35 @@ > +/* > + * virnetdevhostdev.h: utilities to get/verify Switchdev VF Representor > + * > + * > + * 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__ > +#include "virnetdevtap.h" cppi: src/util/virnetdevhostdev.h: line 21: not properly indented cppi: src/util/virnetdevhostdev.h: line 22: not properly indented IOW: "# define" and "# include" > + > +char * > +virNetdevHostdevGetVFRIfName(virDomainHostdevDefPtr hostdev) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; Add a blank like for readability > +bool > +virNetdevHostdevCheckVFRIfName(virDomainHostdevDefPtr hostdev, > + const char *ifname) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; Same here... Also keep the same function order as the .c file - it's just easier that way to compare .h and .c... > +int virNetdevHostdevVFRIfStats(const char *ifname, > + virDomainInterfaceStatsPtr stats, > + bool swapped) Why change format? s/b int virNetdevHostdev... > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > +#endif /* __VIR_NETDEV_HOSTDEV_H__ */ > John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
