Thanks John for a very detailed review and feedback for this patch set. As this is my first patch submission to libvirt, I was really looking for the review from the community and this review is really helpful and informing. I agree with concerns and suggestions highlighted and will prepare v3 to address them. -Jai On Wed, Feb 21, 2018 at 4:00 AM, John Ferlan <jferlan@redhat.com> wrote:

On 02/12/2018 03:07 AM, Jai Singh Rana wrote:

...

Switchdev VF Representor interface name on host is derived based on BDF of pci SR-IOV device in 'hostdev' and querying required net sysfs entries on host.

Really short for what's being added here.

Not sure what BDF is...

s/pci/PCI

Essentially this seems to be an interface for certain hostdev's with certain attributes in order to be able to return specific attributes. Please try to describe a few more details.

Oh and you need a patch 3 to adjust the "docs/news.xml" to describe the enhancement.

BDF refers to BUS:DEVICE:FUNCTION in pci address. Will surely prepare a more informed description in v3 for this feature.

...

--- v2 includes commented code cleanup in virnetdevhostdev.c

po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 5 + src/util/virhostdev.c | 11 +++ src/util/virhostdev.h | 6 ++ src/util/virnetdevhostdev.c | 224 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevhostdev.h | 33 +++++++ 7 files changed, 281 insertions(+) create mode 100644 src/util/virnetdevhostdev.c create mode 100644 src/util/virnetdevhostdev.h

Not in my wheelhouse of knowledge, but I do have some comments...

...

diff --git a/po/POTFILES.in b/po/POTFILES.in index 285955469..73ce73397 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -237,6 +237,7 @@ src/util/virnetdevmacvlan.c src/util/virnetdevmidonet.c src/util/virnetdevopenvswitch.c src/util/virnetdevtap.c +src/util/virnetdevhostdev.c src/util/virnetdevveth.c src/util/virnetdevvportprofile.c src/util/virnetlink.c diff --git a/src/Makefile.am b/src/Makefile.am index db68e01db..0f3c3f1bc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -148,6 +148,7 @@ UTIL_SOURCES = \ util/virnetdev.h util/virnetdev.c \ util/virnetdevbandwidth.h util/virnetdevbandwidth.c \ util/virnetdevbridge.h util/virnetdevbridge.c \ + util/virnetdevhostdev.h util/virnetdevhostdev.c \ util/virnetdevip.h util/virnetdevip.c \ util/virnetdevmacvlan.c util/virnetdevmacvlan.h \ util/virnetdevmidonet.h util/virnetdevmidonet.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b14d7d15..d9bc8ad72 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2288,6 +2288,11 @@ virNetDevBridgeSetSTPDelay; virNetDevBridgeSetVlanFiltering;

+# util/virnetdevhostdev.h +virNetdevHostdevCheckVFRepIFName; +virNetdevHostdevGetVFRepIFName; + + # util/virnetdevip.h virNetDevIPAddrAdd; virNetDevIPAddrDel; diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a12224c58..b6d824d07 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -351,6 +351,17 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, }

+/* Non static wrapper for virHostdevNetDevice to use outside virhostdev */ +int +virHostdevNetDeviceWrapper(virDomainHostdevDefPtr hostdev, + int pfNetDevIdx, + char **linkdev, + int *vf)

1. Arguments have incorrect indentation

2. Why not remove static from virHostdevNetDevice instead? and add it to libvirt_private.syms and add prototype in virhostdev.h?

IOW: I see no need for this wrapper.

I agree this doesn't look good. I was not sure whether removing static from above mentioned function in virhostdev.c is allowed.

...

+{ + return virHostdevNetDevice(hostdev, pfNetDevIdx, linkdev, vf); +} + + static bool virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev) { diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 54e1c66be..7dc860a85 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -202,5 +202,11 @@ int virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, int virHostdevPCINodeDeviceReset(virHostdevManagerPtr mgr, virPCIDevicePtr pci) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virHostdevNetDeviceWrapper(virDomainHostdevDefPtr hostdev, + int pfNetDevIdx, + char **linkdev, + int *vf) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);

^^ If the attributes 1 & 4 for virHostdevNetDevice cannot be NULL, then be sure to add that to the virhostdev.h prototype.

...

#endif /* __VIR_HOSTDEV_H__ */ diff --git a/src/util/virnetdevhostdev.c b/src/util/virnetdevhostdev.c new file mode 100644 index 000000000..243c78a97 --- /dev/null +++ b/src/util/virnetdevhostdev.c

For some reason I have the this is a Linux only warning bells going off... That means for any API that could be called that would be getting some linux specific path you need to have an

#ifdef __linux__ function(args...) { current code }

#else

function(same args ATTRIBUTE_UNUSED,...) { virReportSystemError(ENOSYS, "%s",... ); } #endif

There's plenty of examples that walk /sys/class tree's to pull from - even virNetDevTapInterfaceStats which you've abused later on.

Agreed. As this linux only feature,I should have taken care of this every where in the patch.

...

@@ -0,0 +1,224 @@ +/* + * 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 "virnetdevhostdev.h" +#include "viralloc.h" +#include "virstring.h" +#include "virfile.h" +#include "virerror.h" +#include "virlog.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.netdevhostdev"); + +#ifndef IFNAMSIZ +#define IFNAMSIZ 16

s/#d/# d/

...

+#endif + +#define IFSWITCHIDSIZ 20 + +#ifndef SYSFS_NET_DIR +#define SYSFS_NET_DIR "/sys/class/net/" +#endif

This is defined in virnetdev.h, which should be used here.

...

+ + +/** + * virNetdevHostdevNetSysfsPath + * + * @pf_name: netdev name of the physical function (PF) + * @vf: virtual function (VF) number for the device of interest + * @vf_representor: name of the VF representor interface + * + * Finds the VF representor name of VF# @vf of SRIOV PF @pfname, and + * puts it in @vf_representor. The caller must free @vf_representor + * when it's finished with it + * + * Returns 0 on success, -1 on failure + */ +static int +virNetdevHostdevNetSysfsPath(char *pf_name, int vf, char **vf_representor) +{ + 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_representor_name = NULL; + char *vf_num_str = NULL; + char *vf_suffix = 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;

These look like open coded virNetDevSysfsFile calls...

...

+ + if (virFileReadAllQuiet(pf_switch_id_file, IFSWITCHIDSIZ, + &pf_switch_id) <= 0) {

Since you've used the Quiet function that says you'll supply the error. If you return error here without an error message it'll default to libvirt failed for some reason. What's the reason for failure. Look up other callers to virFileReadAllQuiet

Will take care of this in v3.

...

+ goto cleanup; + } + + 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; +

[1] 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; +

[1] VIR_FREE((pf_subsystem_device_switch_id);

...

+ if (virFileReadAllQuiet(pf_subsystem_device_file, IFSWITCHIDSIZ, + &pf_subsystem_device_switch_id) > 0) { + if (!STREQ(pf_switch_id, pf_subsystem_device_switch_id)) { + VIR_FREE(pf_subsystem_device_file); + VIR_FREE(pf_subsystem_device_switch_id);

[1] Rather than stack these in continue paths, may I suggest moving them to before their usage... Calling VIR_FREE(NULL) is just a side effect of the first time thru the loop.

...

+ continue; + }

Thus the {, VIR_FREE's, and } aren't necessary

...

+ } + + if (virAsprintf(&pf_subsystem_device_port_name_file, + "%s/%s/phys_port_name", pf_subsystem_dir, + dp->d_name) < 0) + goto cleanup; +

[1] VIR_FREE(pf_subsystem_device_port_name_file);

...

+ if (virFileReadAllQuiet + (pf_subsystem_device_port_name_file, IFNAMSIZ, + &vf_representor_name) <= 0) { + VIR_FREE(pf_subsystem_device_file); + VIR_FREE(pf_subsystem_device_switch_id); + VIR_FREE(pf_subsystem_device_port_name_file); + continue; + }

Thus, the {, VIR_FREE's, and } aren't necessary

...

+ + if (virAsprintf(&vf_num_str, "%d", vf) < 0) + goto cleanup; + + /* phys_port_name may contain just VF number or string with + * 'vf' or 'VF' followed by VF number at the end. + */ + if (!(vf_suffix = strcasestr(vf_representor_name, "vf")))

If you had run make syntax-check you would have received an interesting error :

.... avoid_strcase src/util/virnetdevhostdev.c:135: if (!(vf_suffix = strcasestr(vf_representor_name, "vf"))) maint.mk: use c-strcase.h instead of raw strcase functions make: *** [cfg.mk:504: sc_avoid_strcase] Error 1 ...

Searching on strcasestr seems to indicate there could be some problems using it. So the question becomes is this a STRCASENEQLEN(arg, "vf", 2)?

Or does the vf/VF string exist somewhere in the arg?

Either way running syntax-check is a must before your next series.

...

+ vf_suffix = vf_representor_name; + + if (strstr(vf_suffix, vf_num_str)) { + if (virAsprintf(vf_representor, "%s", dp->d_name) < 0) + goto cleanup; + + ret = 0; + break; + } + } + + 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_representor_name); + return ret; +} + + +/** + * virNetdevHostdevGetVFRepIFName + * + * @hostdev: host device to check + * @ifname : Contains VF representor name upon successful return. + * + * Returns 0 on success, -1 on failure + */ +int +virNetdevHostdevGetVFRepIFName(virDomainHostdevDefPtr hostdev, + char **ifname)

Any specific reason to not return @ifname instead? IOW:

No really. ifname can be returned as well.

char * virNetdevHostdevGetVFRepIFName(virDomainHostdevDefPtr hostdev)

In any case, even though currently only called from qemu_driver, having a __linux__ conditional like virNetDevTapInterfaceStats has is a good idea (yes, even though virNetDevOpenvswitchInterfaceStats seems to have escaped similar scrutiny).

...

+{ + char *linkdev = NULL; + char *vf_representor = NULL;

I think it'd be safe with just vf_name, your call on that though.

...

+ int vf = -1; + int ret = -1; + + if (virHostdevNetDeviceWrapper(hostdev, -1, &linkdev, &vf) < 0) + goto cleanup; + + if (virNetdevHostdevNetSysfsPath(linkdev, vf, &vf_representor)) + goto cleanup;

Since you'd be getting a /sys/class/net path here, that's why you'd need to have a non __linux__ API...

...

+ + if (VIR_STRDUP(*ifname, vf_representor) > 0) + ret = 0; + + cleanup: + VIR_FREE(linkdev); + VIR_FREE(vf_representor); + return ret; +} + + +/** + * virNetdevHostdevCheckVFRepIFName + * + * @hostdev: host device to check + * @ifname : VF representor name to verify + * + * Returns 0 on success, -1 on failure

This seemingly could be a boolean function - there's no checking going on other than equality, e.g. virNetdevHostdevIsVFRep(hostdev, ifname)

Again, you'll need a __linux__ conditional for this function...

...

+ */ +int +virNetdevHostdevCheckVFRepIFName(virDomainHostdevDefPtr hostdev, + const char *ifname) +{ + char *linkdev = NULL; + char *vf_representor = NULL;

Similar, I think vf_ifname is fine.

...

+ int vf = -1; + int ret = -1;

bool ret = false;

...

+ + if (virHostdevNetDeviceWrapper(hostdev, -1, &linkdev, &vf) < 0) + goto cleanup; + + if (virNetdevHostdevNetSysfsPath(linkdev, vf, &vf_representor)) + goto cleanup; + + if (STREQ(ifname, vf_representor)) + ret = 0;

ret = STREQ(ifname, vf_ifname);

...

+ + cleanup: + VIR_FREE(linkdev); + VIR_FREE(vf_representor); + return ret; +} diff --git a/src/util/virnetdevhostdev.h b/src/util/virnetdevhostdev.h new file mode 100644 index 000000000..c79c697fd --- /dev/null +++ b/src/util/virnetdevhostdev.h @@ -0,0 +1,33 @@ +/* + * 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"

Based on below, this won't be necessary either.

...

+ +int +virNetdevHostdevGetVFRepIFName(virDomainHostdevDefPtr hostdev, + char **ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int +virNetdevHostdevCheckVFRepIFName(virDomainHostdevDefPtr hostdev, + const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +#define virNetdevHostdevVFRepInterfaceStats virNetDevTapInterfaceStats

This #define doesn't look right... I smell a hack coming.

John

...

+#endif /* __VIR_NETDEV_HOSTDEV_H__ */

-Jai

On 02/12/2018 03:07 AM, Jai Singh Rana wrote:

In case of <interface type='hostdev'>, return stats if its a Switchdev VF Representor interface of pci SR-IOV device. --- v2 fixes bracket spacing in domain_conf.c

src/conf/domain_conf.c | 7 +++++++ src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fb732a0c2..b553c5a2f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -58,6 +58,7 @@ #include "virnetdev.h" #include "virnetdevmacvlan.h" #include "virhostdev.h" +#include "virnetdevhostdev.h" #include "virmdev.h"

#define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -28112,6 +28113,12 @@ virDomainNetFind(virDomainDefPtr def, const char *device) return net; }

+ /* Give a try to hostdev */ + for (i = 0; i < def->nnets; i++) {

If there's 10 nnets and 1 nhostdev, things are not going to end well.

+ if (!virNetdevHostdevCheckVFRepIFName(def->hostdevs[i], device)) + return def->nets[i];

Wait, what?

+ } +

Even w/ the correct usage - there's more than just one caller that could get an answer it wasn't expecting... Limit your usage to where you need this type of check...

virReportError(VIR_ERR_INVALID_ARG, _("'%s' is not a known interface"), device); return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bbce5bd81..24484ab92 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -67,6 +67,7 @@ #include "virhostcpu.h" #include "virhostmem.h" #include "virnetdevtap.h" +#include "virnetdevhostdev.h" #include "virnetdevopenvswitch.h" #include "capabilities.h" #include "viralloc.h" @@ -11153,6 +11154,11 @@ 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) { + if (virNetdevHostdevVFRepInterfaceStats(device, stats, + !virDomainNetTypeSharesHostView + (net)) < 0) + goto cleanup;

So you've hidden virNetdevHostdevVFRepInterfaceStats as a call to virNetDevTapInterfaceStats via the #define in virnetdevhostdev.h You need to figure out a different, better, more standard, non-hack mechanism for this. Rather than the virDomainNetFind adjustment above, this is where you should be more explicit.

} else { if (virNetDevTapInterfaceStats(net->ifname, stats, !virDomainNetTypeSharesHostView(net)) < 0) @@ -19794,6 +19800,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, { size_t i; struct _virDomainInterfaceStats tmp; + char *vf_representor_ifname = NULL;

Can we go with just vf_ifname?

int ret = -1;

if (!virDomainObjIsActive(dom)) @@ -19806,21 +19813,40 @@ 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) { + if (virNetdevHostdevGetVFRepIFName(dom->def->hostdevs[i], + &vf_representor_ifname))

Either this checks "< 0)" or if changed such that the called function returns some string...

+ 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, net->ifname); + else + QEMU_ADD_NAME_PARAM(record, maxparams, + "net", "name", i, vf_representor_ifname);

Note that QEMU_ADD_NAME_PARAM can goto cleanup, thus leaking your vf*_ifname. Just add the VIR_FREE at the cleanup label

if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) { virResetLastError(); continue; } + } else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + if (virNetdevHostdevVFRepInterfaceStats + (vf_representor_ifname, &tmp, + !virDomainNetTypeSharesHostView(net)) < 0) { + VIR_FREE(vf_representor_ifname); + virResetLastError(); + continue; + } + VIR_FREE(vf_representor_ifname);

int rc; rc = virNetdevHostdevVFRepInterfaceStats(vf_iname, &tmp, !virDomainNetTypeSharesHostView(net)) VIR_FREE(vf_iname); if (rc < 0) { virResetLastError(); continue; } is a bit cleaner to me John

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