Re: [libvirt] [PATCH v2 2/2] qemu: conf: Network stats support for hostdev VF Representor

2018-02-21 Thread Jai Singh Rana 
Again thanks for the feedback for this patch set. Helps me a lot. Will
take care feedback
 in v3.

On Wed, Feb 21, 2018 at 4:00 AM, John Ferlan  wrote:
>
>
> On 02/12/2018 03:07 AM, Jai Singh Rana wrote:
>> In case of , 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?
>
>> +}
>> +
>

Agree. I need to iterate over nhostdevs as well to map prpoerly.

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

Yes it doesn't look good. I wasn't sure whether this was even allowed
but used it to avoid
duplicate code for virNetDevTapInterfaceStats in virnetdevhostdev.c
Need to take care of this in v3.

>
>>  } 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],
>> +   _representor_ifname))
>
> Either this checks "< 0)" or if changed such that the called function
> returns some string...
>
>> +continue;
>> +}
>> +else if (!net->ifname)
>>  continue;
>>
>>  memset(, 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);
>
>

Re: [libvirt] [PATCH v2 2/2] qemu: conf: Network stats support for hostdev VF Representor

2018-02-20 Thread John Ferlan 


On 02/12/2018 03:07 AM, Jai Singh Rana wrote:
> In case of , 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],
> +   _representor_ifname))

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

> +continue;
> +}
> +else if (!net->ifname)
>  continue;
>  
>  memset(, 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, ) < 0) {
>  virResetLastError();
>  continue;
>  }
> +} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +if (virNetdevHostdevVFRepInterfaceStats
> +(vf_representor_ifname, ,
> +

[libvirt] [PATCH v2 2/2] qemu: conf: Network stats support for hostdev VF Representor

2018-02-12 Thread Jai Singh Rana 
In case of , 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 (!virNetdevHostdevCheckVFRepIFName(def->hostdevs[i], device))
+return def->nets[i];
+}
+
 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;
 } 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;
 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],
+   _representor_ifname))
+continue;
+}
+else if (!net->ifname)
 continue;
 
 memset(, 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);
 
 if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
 if (virNetDevOpenvswitchInterfaceStats(net->ifname, ) < 0) {
 virResetLastError();
 continue;
 }
+} else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+if (virNetdevHostdevVFRepInterfaceStats
+(vf_representor_ifname, ,
+ !virDomainNetTypeSharesHostView(net)) < 0) {
+VIR_FREE(vf_representor_ifname);
+virResetLastError();
+continue;
+}
+VIR_FREE(vf_representor_ifname);
 } else {
 if (virNetDevTapInterfaceStats(net->ifname, ,

!virDomainNetTypeSharesHostView(net)) < 0) {
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list