On 09/11/2012 08:15 PM, Laine Stump wrote: > (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? Thanks for the review Laine. I had actually encountered this issue when I was working on the patches but at that moment I could not think of a workaround. Thanks for the suggestion. I will do the needful. > >> + 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list