[libvirt] [PATCH v4 4/6] conf: util: Add API to find net def given its domain's hostdev

[John Ferlan]

$SUBJ:

"conf: Introduce virDomainNetFindByHostdev"

Is fine.

then the commit message can be more descriptive.

On 06/28/2018 09:22 AM, Jai Singh Rana wrote:
> Introduce API virDomainNetFindByHostdev which retrieves net def in
> given domain for the given hostdev def. This API will now be used by
> virDomainNetFind to further probe net for hostdev network devices.
> 
> Signed-off-by: Jai Singh Rana <JaiSingh.Rana at cavium.com>
> ---
>  src/conf/domain_conf.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |  2 ++
>  src/libvirt_private.syms |  2 ++
>  src/util/virhostdev.c    |  2 +-
>  src/util/virhostdev.h    |  3 +++
>  5 files changed, 51 insertions(+), 1 deletion(-)

You could split out the last 3 into their own patch to promote
virHostdevIsPCINetDevice from static to global, but that's just a nit.

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8cb7f37f3..8432215d19 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -56,6 +56,7 @@
>  #include "virsecret.h"
>  #include "virstring.h"
>  #include "virnetdev.h"
> +#include "virnetdevhostdev.h"
>  #include "virnetdevmacvlan.h"
>  #include "virhostdev.h"
>  #include "virmdev.h"
> @@ -29064,6 +29065,37 @@ virDomainNetFind(virDomainDefPtr def, const char *device)
>  
>  
>  /**
> + * virDomainNetFindByHostdev:
> + * @def: domain's def
> + * @hostdev: hostdev whose net def if exists to be retrieved
> + *
> + * Finds net def in domain given the domain's hostdev.
> + *
> + * Returns a pointer to the net def or NULL if not found.
> + */
> +virDomainNetDefPtr
> +virDomainNetFindByHostdev(virDomainDefPtr def,
> +                          virDomainHostdevDefPtr hostdev)
> +{
> +    size_t i;
> +    virDomainNetType actualType;
> +    virDomainHostdevDefPtr hostdef = NULL;
> +
> +    for (i = 0; i < def->nnets; i++) {
> +         actualType = virDomainNetGetActualType(def->nets[i]);
> +         if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV)
> +             hostdef = virDomainNetGetActualHostdev(def->nets[i]);

Of concern, virDomainNetGetActualHostdev can return NULL...

> +         else
> +             continue;
> +         if  (!memcmp(hostdev, hostdef, sizeof(virDomainHostdevDef)))

Is comparing the whole thing necessary? Is perhaps just the name good
enough? There's a lot of data in the HostdevDef - if something doesn't
match then I suppose you could conceivably fail here even though you're
not expecting to.

> +              return def->nets[i];
> +    }

Does the following work?

    for (i = 0; i < def->nnets; i++) {
        virDomainHostdevDefPtr net_hostdev;

        if (!(net_hostdev = virDomainNetGetActualHostdev(def->nets[i])))
            continue;

        [whatever comparison is appropriate for whether net_hostdev
matches the passed hostdev]

> +
> +    return NULL;
> +}
> +
> +
> +/**
>   * virDomainNetFindByName:
>   * @def: domain's def
>   * @ifname: interface name
> @@ -29077,12 +29109,23 @@ virDomainNetFindByName(virDomainDefPtr def,
>                         const char *ifname)
>  {
>      size_t i;
> +    virDomainNetDefPtr net = NULL;
>  
>      for (i = 0; i < def->nnets; i++) {
>          if (STREQ_NULLABLE(ifname, def->nets[i]->ifname))
>              return def->nets[i];
>      }
>  
> +    /* Give a try to hostdev if its a switchdev network device*/

s/its/it's/

s/device*/device */

> +    for (i = 0; i < def->nhostdevs; i++) {
> +         if (!virHostdevIsPCINetDevice(def->hostdevs[i]))
> +             continue;
> +         if (virNetdevHostdevCheckVFRIfName(def->hostdevs[i], ifname)) {
> +             if ((net = virDomainNetFindByHostdev(def, def->hostdevs[i])))
> +                 return net;
> +         }
> +    }
> +

IIUC, this is the "magic" place where as stated in the doc patch6 where
the hostdev being used to fetch the stats is determined to be an SR-IOV
device w/ VF representor on host in switchdev mode.

There is some concern over adding this without some sort of "filter" -
mainly because of the extra work, but also because it's not 100% clear
to me all callers would expect to get this data.

Anyway, for callers of virDomainNetFind that end up just calling
virNetDevTapInterfaceStats - it would seem to be OK, but I'm not 100%
sure. For callers that are getting/setting interface parameters - would
returning the @net that is actually a hostdev type be expected?

Maybe it'd be best to introduce a boolean 'wantSRIOVHostdevVFR' that
would be "true" only from qemuDomainInterfaceStats since it then has the
logic to detect if the ActualType(net) is HOSTDEV (name chosen from my
IIUC above - feel free to adjust to match reality).

That way we only check the hostdev's if that's what we want with the
logic being:

    if (!wantSRIOVHostdevVFR)
        return NULL;

    for (i = 0; i < def->nhostdevs; i++) {
    ...

John

>      return NULL;
>  }
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0924fc4f3c..ccec74e51d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3146,6 +3146,8 @@ int virDomainDiskSourceParse(xmlNodePtr node,
>  
>  int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
>  virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device);
> +virDomainNetDefPtr virDomainNetFindByHostdev(virDomainDefPtr def,
> +                                             virDomainHostdevDefPtr hostdev);
>  virDomainNetDefPtr virDomainNetFindByName(virDomainDefPtr def, const char *ifname);
>  bool virDomainHasNet(virDomainDefPtr def, virDomainNetDefPtr net);
>  int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5aa8f7ed64..e4d8583d41 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -449,6 +449,7 @@ virDomainNetDefClear;
>  virDomainNetDefFormat;
>  virDomainNetDefFree;
>  virDomainNetFind;
> +virDomainNetFindByHostdev;
>  virDomainNetFindByName;
>  virDomainNetFindIdx;
>  virDomainNetGenerateMAC;
> @@ -1966,6 +1967,7 @@ virHostCPUStatsAssign;
>  # util/virhostdev.h
>  virHostdevFindUSBDevice;
>  virHostdevIsMdevDevice;
> +virHostdevIsPCINetDevice;
>  virHostdevIsSCSIDevice;
>  virHostdevManagerGetDefault;
>  virHostdevNetDevice;
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index c6ee725860..2b8ecb9649 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -348,7 +348,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
>  }
>  
>  
> -static bool
> +bool
>  virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev)
>  {
>      return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h
> index 7412a20aa9..71faaf4e7a 100644
> --- a/src/util/virhostdev.h
> +++ b/src/util/virhostdev.h
> @@ -197,6 +197,9 @@ virHostdevReAttachDomainDevices(virHostdevManagerPtr mgr,
>                                  const char *oldStateDir)
>      ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>  bool
> +virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev)
> +    ATTRIBUTE_NONNULL(1);
> +bool
>  virHostdevIsSCSIDevice(virDomainHostdevDefPtr hostdev)
>      ATTRIBUTE_NONNULL(1);
>  bool
>