$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@xxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
