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

[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 <jferlan at 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