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 (!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],
+ &vf_representor_ifname))
+ 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);
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);
} else {
if (virNetDevTapInterfaceStats(net->ifname, &tmp,
!virDomainNetTypeSharesHostView(net)) < 0) {
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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) {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 <jferlan@redhat.com> wrote:
>
>
> 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?
>
>> + }
>> +
>
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],
>> + &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) {
>>
Sure. I agree.
-Jai
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.