Re: [libvirt] [PATCH v2 1/2] util: Add helper APIs to get/verify VF Representor name

[Jai Singh Rana]

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

Re: [libvirt] [PATCH v2 1/2] util: Add helper APIs to get/verify VF Representor name

[John Ferlan]

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 0..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 @@
> +/*
> + * 

[libvirt] [PATCH v2 1/2] util: Add helper APIs to get/verify VF Representor name

[Jai Singh Rana]

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.
---
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

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)
+{
+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);
 
 #endif /* __VIR_HOSTDEV_H__ */
diff --git a/src/util/virnetdevhostdev.c b/src/util/virnetdevhostdev.c
new file mode 100644
index 0..243c78a97
--- /dev/null
+++ b/src/util/virnetdevhostdev.c
@@ -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
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#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
+#endif
+
+#define IFSWITCHIDSIZ 20
+
+#ifndef SYSFS_NET_DIR
+#define SYSFS_NET_DIR "/sys/class/net/"
+#endif
+
+
+/**
+ * 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