Re: [libvirt] [PATCH 3/8] Hostdev-hybrid mode requires a direct linkdev and direct mode.

(not a full review yet, but a couple of things I noticed in passing) On 09/07/2012 12:15 PM, Shradha Shah wrote: > In this mode the guest contains a Virtual network device along with a > SRIOV VF passed through to the guest as a pci device. > --- > src/conf/domain_conf.c | 38 ++++++++++++++++++++++++++++++++++++-- > src/conf/domain_conf.h | 5 +++++ > src/libvirt_private.syms | 1 + > src/util/pci.c | 2 +- > src/util/pci.h | 2 ++ > src/util/virnetdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ > src/util/virnetdev.h | 6 ++++++ > 7 files changed, 91 insertions(+), 3 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index d8ab40c..c59ea00 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1022,6 +1022,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) > virDomainHostdevDefClear(&def->data.hostdev.def); > break; > case VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID: > + VIR_FREE(def->data.hostdev.linkdev); > virDomainHostdevDefClear(&def->data.hostdev.def); > break; > default: > @@ -1077,6 +1078,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) > break; > > case VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID: > + VIR_FREE(def->data.hostdev.linkdev); > virDomainHostdevDefClear(&def->data.hostdev.def); > break; > > @@ -4687,6 +4689,7 @@ virDomainNetDefParseXML(virCapsPtr caps, > char *mode = NULL; > char *linkstate = NULL; > char *addrtype = NULL; > + char *pfname = NULL; > virNWFilterHashTablePtr filterparams = NULL; > virDomainActualNetDefPtr actual = NULL; > xmlNodePtr oldnode = ctxt->node; > @@ -5024,6 +5027,27 @@ virDomainNetDefParseXML(virCapsPtr caps, > hostdev, flags) < 0) { > goto error; > } > + > + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { > + if (virNetDevGetPhysicalFunctionFromVfPciAddr(hostdev->source.subsys.u.pci.domain, > + hostdev->source.subsys.u.pci.bus, > + hostdev->source.subsys.u.pci.slot, > + hostdev->source.subsys.u.pci.function, > + &pfname) < 0) { It's problematic to have this call in a parsing function - it requires the actual hardware to be present on the machine doing the parse. For example, doing this in the parse causes the qemuxml2xml and qemuxml2argv tests to fail on my system, because my hardware doesn't match yours: One last thing - after applying the entire series, I'm getting the following failures in make check: VIR_TEST_DEBUG=2 ./qemuxml2xmltest TEST: qemuxml2xmltest [...] 7) QEMU XML-2-XML net-hostdevhybrid ... libvir: Domain Config error : internal error Could not get Physical Function of the hostdev ... VIR_TEST_DEBUG=2 ./qemuxml2argvtest TEST: qemuxml2argvtest [...] 113) QEMU XML-2-ARGV net-hostdevhybrid ... libvir: Domain Config error : internal error Could not get Physical Function of the hostdev I couldn't find any other uses of network device "ioctl" type calls in the conf directory. I think we at least frown on doing that in parsing/formatting, and may even forbid it. At any rate, You need to not do this here. Instead, perhaps you can leave it empty if it's not given explicitly in the input XML, and fill it in in the caller when appropriate?

> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not get Physical Function of the hostdev")); > + goto error; > + } > + } > + if (pfname != NULL) > + def->data.hostdev.linkdev = strdup(pfname); > + else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Linkdev is required in %s mode"), > + virDomainNetTypeToString(def->type)); > + goto error; > + } > + def->data.hostdev.mode = VIR_NETDEV_MACVLAN_MODE_BRIDGE; > break; > > case VIR_DOMAIN_NET_TYPE_USER: > @@ -14664,11 +14688,16 @@ virDomainNetGetActualDirectDev(virDomainNetDefPtr iface) > { > if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT) > return iface->data.direct.linkdev; > + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID) > + return iface->data.hostdev.linkdev; > if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) > return NULL; > if (!iface->data.network.actual) > return NULL; > - return iface->data.network.actual->data.direct.linkdev; > + if (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID) > + return iface->data.network.actual->data.hostdev.linkdev; > + else > + return iface->data.network.actual->data.direct.linkdev; > } > > int > @@ -14676,11 +14705,16 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr iface) > { > if (iface->type == VIR_DOMAIN_NET_TYPE_DIRECT) > return iface->data.direct.mode; > + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID) > + return iface->data.hostdev.mode; > if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) > return 0; > if (!iface->data.network.actual) > return 0; > - return iface->data.network.actual->data.direct.mode; > + if (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV_HYBRID) > + return iface->data.network.actual->data.hostdev.mode; > + else > + return iface->data.network.actual->data.direct.mode; > } > > virDomainHostdevDefPtr > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 156eb32..171dd70 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -44,6 +44,7 @@ > # include "virnetdevopenvswitch.h" > # include "virnetdevbandwidth.h" > # include "virnetdevvlan.h" > +# include "virnetdev.h" > # include "virobject.h" > # include "device_conf.h" > > @@ -778,6 +779,8 @@ struct _virDomainActualNetDef { > int mode; /* enum virMacvtapMode from util/macvtap.h */ > } direct; > struct { > + char *linkdev; > + int mode; It appears that the mode is always "bridge", so I don't see any point in including it here. > virDomainHostdevDef def; > } hostdev; > } data; > @@ -833,6 +836,8 @@ struct _virDomainNetDef { > int mode; /* enum virMacvtapMode from util/macvtap.h */ > } direct; > struct { > + char *linkdev; > + int mode; > virDomainHostdevDef def; > } hostdev; > } data; > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 10af063..2d06cc3 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1399,6 +1399,7 @@ virNetDevGetMTU; > virNetDevGetPhysicalFunction; > virNetDevGetVLanID; > virNetDevGetVirtualFunctionIndex; > +virNetDevGetPhysicalFunctionFromVfPciAddr; > virNetDevGetVirtualFunctionInfo; > virNetDevGetVirtualFunctions; > virNetDevIsOnline; > diff --git a/src/util/pci.c b/src/util/pci.c > index 0742d07..ba8fffc 100644 > --- a/src/util/pci.c > +++ b/src/util/pci.c > @@ -802,7 +802,7 @@ pciDriverFile(char **buffer, const char *driver, const char *file) > return 0; > } > > -static int > +int > pciDeviceFile(char **buffer, const char *device, const char *file) > { > VIR_FREE(*buffer); > diff --git a/src/util/pci.h b/src/util/pci.h > index 8bbab07..936fee4 100644 > --- a/src/util/pci.h > +++ b/src/util/pci.h > @@ -116,6 +116,8 @@ int pciConfigAddressToSysfsFile(struct pci_config_address *dev, > > int pciDeviceNetName(char *device_link_sysfs_path, char **netname); > > +int pciDeviceFile(char **buffer, const char *device, const char *file); > + > int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) > ATTRIBUTE_RETURN_CHECK; > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index f622f5d..4d4fcb1 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -1097,6 +1097,46 @@ virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname, > } > > /** > + * virNetDevGetPhyscialFunctionFromVfPciAddr > + * > + * @domain : Domain part of VF PCI addr > + * @bus : Bus part of VF PCI addr > + * @slot : Slot part of VF PCI addr > + * @function : Function part of VF PCI addr > + * @pfname : Contains sriov physical function for Vf upon > + * successful return > + * > + * Returns 0 on success, -1 on failure > + * > + */ > +int > +virNetDevGetPhysicalFunctionFromVfPciAddr(unsigned domain, > + unsigned bus, > + unsigned slot, > + unsigned function, > + char **pfname) > +{ > + char *pciConfigAddr = NULL; > + char *physfn_sysfs_path = NULL; > + int ret = -1; > + > + if (pciGetDeviceAddrString(domain, bus, slot, function, > + &pciConfigAddr) < 0) { > + goto cleanup; > + } > + if (pciDeviceFile(&physfn_sysfs_path, pciConfigAddr, "physfn") < 0) { > + goto cleanup; > + } > + ret = pciDeviceNetName(physfn_sysfs_path, pfname); > + > +cleanup: > + VIR_FREE(pciConfigAddr); > + VIR_FREE(physfn_sysfs_path); > + > + return ret; > +} > + > +/** > * virNetDevGetPhysicalFunction > * > * @ifname : name of the physical function interface name > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index 705ad9c..2e102f2 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -99,6 +99,12 @@ int virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname, > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > ATTRIBUTE_RETURN_CHECK; > > +int virNetDevGetPhysicalFunctionFromVfPciAddr(unsigned domain, unsigned bus, > + unsigned slot, unsigned function, > + char **pfname) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; > + > int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list