[libvirt] [PATCH v2 1/2] util: Add helper APIs to get/verify VF Representor name
John Ferlan
jferlan at redhat.com
Tue Feb 20 22:30:27 UTC 2018
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.
> ---
> 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.
> +{
> + 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.
> @@ -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
> + 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:
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__ */
>
More information about the libvir-list
mailing list