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@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
