[libvirt] [PATCH 0/5 v3] Interface pools and passthrough mode
Interface Pools and Passthrough mode: Current Method: The passthrough mode uses a macvtap direct connection to connect each guest to the network. The physical interface to be used is picked from among those listed in <interface> sub elements of the <forward> element. The current specification for <forward> extends to allow 0 or more <interface> sub-elements: Example: <forward mode='passthrough' dev='eth10'/> <interface dev='eth10'/> <interface dev='eth12'/> <interface dev='eth18'/> <interface dev='eth20'/> </forward> However with an ethernet card with 64 VF's or more, the above method gets tedious on the system. On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are multiple non-contiguous ranges. Proposed Method: The 4 patches provided: i) Introduce a new element 'pf' to implicitly create an interface pool of all the Virtual Functions attached to the specified Physical Function. ii) Modify the networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice API to use the above mentioned interface pool in the passthrough mode. iii) Allow virsh net-dumpxml to use an option --inactive to differentiate between explicit and implicit interface pools Hence Libvirt will now support both the methods mentioned below: * Explicit interface list. App inputs: <forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward> libvirt does not change XML * Automatically interface list from PF. App inputs: <forward mode='passthrough'> <pf dev='eth0'/> </forward> libvirt expands XML to be <forward mode='passthrough'> <pf dev='eth0'/> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward> In the above case we need to differentiate between the implicit and explicit interface pool, which can be done by comparing the dumpxml from active and inactive domains. This will need the addition of the flag VIR_NETWORK_XML_INACTIVE to virNetworkGetXMLDesc(). This patch series supports the use of option --inactive with virsh net-dumpxml. Shradha Shah (5): Added function pciSysfsFile to enable access to the PCI SYSFS files. Added Function virNetDevGetVirtualFunctions Adding the element pf to network xml. Functionality to implicitly get interface pool from SR-IOV PF. Added new option to virsh net-dumpxml called --inactive docs/schemas/network.rng | 7 +++ include/libvirt/libvirt.h.in | 4 ++ src/conf/network_conf.c | 79 +++++++++++++++++++++++++++++++--- src/conf/network_conf.h | 5 ++- src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++--------- src/test/test_driver.c | 2 +- src/util/pci.c | 39 +++++++++++++++++ src/util/pci.h | 7 +++ src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++- 13 files changed, 311 insertions(+), 35 deletions(-) -- 1.7.4.4
--- src/util/pci.c | 16 ++++++++++++++++ src/util/pci.h | 2 ++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index cd82b43..d0ba4c5 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -2027,6 +2027,22 @@ out: } /* + * Returns a path to the PCI sysfs file given the BDF of the PCI function + */ + +int +pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) +{ + if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s", + pciDeviceName) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} + +/* * Returns the network device name of a pci device */ int diff --git a/src/util/pci.h b/src/util/pci.h index 76e37e3..f98e745 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -109,4 +109,6 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link, int pciDeviceNetName(char *device_link_sysfs_path, char **netname); +int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link); + #endif /* __VIR_PCI_H__ */ -- 1.7.4.4
On Wed, Dec 14, 2011 at 10:50:01AM +0000, Shradha Shah wrote:
--- src/util/pci.c | 16 ++++++++++++++++ src/util/pci.h | 2 ++ 2 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index cd82b43..d0ba4c5 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -2027,6 +2027,22 @@ out: }
/* + * Returns a path to the PCI sysfs file given the BDF of the PCI function + */ + +int +pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) +{ + if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s", + pciDeviceName) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} + +/* * Returns the network device name of a pci device */ int diff --git a/src/util/pci.h b/src/util/pci.h index 76e37e3..f98e745 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -109,4 +109,6 @@ int pciGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
+int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link);
We could add the 'ATTRIBUTE_RETURN_CHECK' annotation to this method
#endif /* __VIR_PCI_H__ */
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/10/2012 03:50 AM, Daniel P. Berrange wrote:
On Wed, Dec 14, 2011 at 10:50:01AM +0000, Shradha Shah wrote:
--- src/util/pci.c | 16 ++++++++++++++++ src/util/pci.h | 2 ++ 2 files changed, 18 insertions(+), 0 deletions(-)
In addition to Daniel's comments,
+int +pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link) +{ + if (virAsprintf(pci_sysfs_device_link, PCI_SYSFS "devices/%s", + pciDeviceName) < 0) { + virReportOOMError(); + return -1; + } +
Trailing whitespace. 'make syntax-check' caught this.
+int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link); We could add the 'ATTRIBUTE_RETURN_CHECK' annotation to this method
#endif /* __VIR_PCI_H__ */
ACK
I've got some time to apply the necessary minor fixes, so I'll push the corrected series in the next hour or so. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This functions enables us to get the Virtual Functions attached to a Physical function given the name of a SR-IOV physical functio. In order to accomplish the task, added a getter function pciGetDeviceAddrString to get the BDF of the Virtual Function in a char array. --- src/util/pci.c | 23 ++++++++++++++ src/util/pci.h | 5 +++ src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 4 files changed, 117 insertions(+), 0 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index d0ba4c5..ca05e8f 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1293,6 +1293,29 @@ pciReadDeviceID(pciDevice *dev, const char *id_name) return id_str; } +int +pciGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + char **pciConfigAddr) +{ + pciDevice *dev = NULL; + int ret = -1; + + dev = pciGetDevice(domain, + bus, + slot, + function); + if (dev != NULL) { + *pciConfigAddr = strdup(dev->name); + ret = 0; + } + + pciFreeDevice(dev); + return ret; +} + pciDevice * pciGetDevice(unsigned domain, unsigned bus, diff --git a/src/util/pci.h b/src/util/pci.h index f98e745..b7723b1 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -111,4 +111,9 @@ int pciDeviceNetName(char *device_link_sysfs_path, char **netname); int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link); +int pciGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + char **pciConfigAddr); #endif /* __VIR_PCI_H__ */ diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index fee87ef..3bbb76e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -970,6 +970,78 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname, } /** + * virNetDevGetVirtualFunctions: + * + * @pfname : name of the physical function interface name + * @vfname: array that will hold the interface names of the virtual_functions + * @n_vfname: pointer to the number of virtual functions + * + * Returns 0 on success and -1 on failure + */ + +int +virNetDevGetVirtualFunctions(const char *pfname, + char ***vfname, + unsigned int *n_vfname) +{ + int ret = -1, i; + char *pf_sysfs_device_link = NULL; + char *pci_sysfs_device_link = NULL; + struct pci_config_address **virt_fns; + char *pciConfigAddr; + + if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) + return ret; + + if (pciGetVirtualFunctions(pf_sysfs_device_link, &virt_fns, + n_vfname) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get virtual functions")); + goto out; + } + + if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) { + virReportOOMError(); + goto out; + } + + for (i = 0; i < *n_vfname; i++) + { + if (pciGetDeviceAddrString(virt_fns[i]->domain, + virt_fns[i]->bus, + virt_fns[i]->slot, + virt_fns[i]->function, + &pciConfigAddr) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get PCI Config Address String")); + goto out; + } + if (pciSysfsFile(pciConfigAddr, &pci_sysfs_device_link) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get PCI SYSFS file")); + goto out; + } + + if (pciDeviceNetName(pci_sysfs_device_link, &((*vfname)[i])) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get interface name of the VF")); + goto out; + } + } + + ret = 0; + +out: + for (i = 0; i < *n_vfname; i++) + VIR_FREE(virt_fns[i]); + VIR_FREE(virt_fns); + VIR_FREE(pf_sysfs_device_link); + VIR_FREE(pci_sysfs_device_link); + VIR_FREE(pciConfigAddr); + return ret; +} + +/** * virNetDevIsVirtualFunction: * @ifname : name of the interface * @@ -1056,6 +1128,17 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname) return ret; } #else /* !__linux__ */ + +int +virNetDevGetVirtualFunctions(const char *pfname ATTRIBUTE_UNUSED, + char ***vfname ATTRIBUTE_UNUSED, + unsigned int *n_vfname ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get virtual functions on this platfornm")); + return -1; +} + int virNetDevIsVirtualFunction(const char *ifname ATTRIBUTE_UNUSED) { diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 13ba1da..0a885d5 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -99,4 +99,10 @@ int virNetDevGetVirtualFunctionIndex(const char *pfname, const char *vfname, int virNetDevGetPhysicalFunction(const char *ifname, char **pfname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevGetVirtualFunctions(const char *pfname, + char ***vfname, + unsigned int *n_vfname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_H__ */ -- 1.7.4.4
On Wed, Dec 14, 2011 at 10:50:14AM +0000, Shradha Shah wrote:
This functions enables us to get the Virtual Functions attached to a Physical function given the name of a SR-IOV physical functio.
In order to accomplish the task, added a getter function pciGetDeviceAddrString to get the BDF of the Virtual Function in a char array. --- src/util/pci.c | 23 ++++++++++++++ src/util/pci.h | 5 +++ src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ 4 files changed, 117 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index d0ba4c5..ca05e8f 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1293,6 +1293,29 @@ pciReadDeviceID(pciDevice *dev, const char *id_name) return id_str; }
+int +pciGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + char **pciConfigAddr) +{ + pciDevice *dev = NULL; + int ret = -1; + + dev = pciGetDevice(domain, + bus, + slot, + function); + if (dev != NULL) { + *pciConfigAddr = strdup(dev->name);
We need to raise virReportOOMError() on failure here
+ ret = 0; + } + + pciFreeDevice(dev); + return ret; +} +
There is trailing whitespace on several lines in this method, and others in this patch If you run 'make syntax-check' it should tell you of all the problem areas.
pciDevice * pciGetDevice(unsigned domain, unsigned bus, diff --git a/src/util/pci.h b/src/util/pci.h index f98e745..b7723b1 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -111,4 +111,9 @@ int pciDeviceNetName(char *device_link_sysfs_path, char **netname);
int pciSysfsFile(char *pciDeviceName, char **pci_sysfs_device_link);
+int pciGetDeviceAddrString(unsigned domain, + unsigned bus, + unsigned slot, + unsigned function, + char **pciConfigAddr);
Can add ATTRIBUTE_RETURN_CHECK to this one too
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index fee87ef..3bbb76e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -970,6 +970,78 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname, }
/** + * virNetDevGetVirtualFunctions: + * + * @pfname : name of the physical function interface name + * @vfname: array that will hold the interface names of the virtual_functions + * @n_vfname: pointer to the number of virtual functions + * + * Returns 0 on success and -1 on failure + */ + +int +virNetDevGetVirtualFunctions(const char *pfname, + char ***vfname, + unsigned int *n_vfname) +{ + int ret = -1, i; + char *pf_sysfs_device_link = NULL; + char *pci_sysfs_device_link = NULL; + struct pci_config_address **virt_fns; + char *pciConfigAddr; + + if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) + return ret; + + if (pciGetVirtualFunctions(pf_sysfs_device_link, &virt_fns, + n_vfname) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get virtual functions"));
You can remove this virReportSystemError call here, because pciGetVirtualFunctions() will already raise an error on failure
+ goto out; + } + + if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) { + virReportOOMError(); + goto out; + }
Since we've allocated '*vfname' at this point, we need to make sure that we do 'VIR_FREE(vfname)' in any error paths.
+ + for (i = 0; i < *n_vfname; i++) + { + if (pciGetDeviceAddrString(virt_fns[i]->domain, + virt_fns[i]->bus, + virt_fns[i]->slot, + virt_fns[i]->function, + &pciConfigAddr) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get PCI Config Address String")); + goto out; + } + if (pciSysfsFile(pciConfigAddr, &pci_sysfs_device_link) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get PCI SYSFS file")); + goto out; + } + + if (pciDeviceNetName(pci_sysfs_device_link, &((*vfname)[i])) < 0) { + virReportSystemError(ENOSYS, "%s", + _("Failed to get interface name of the VF")); + goto out; + } + } + + ret = 0; + +out:
Our convention for goto label naming suggests 'cleanup' as the name rather than 'out' (yes, I know not all of our code is in compliance with this, so you might have seen other cases of 'out' elsewhre).
+ for (i = 0; i < *n_vfname; i++) + VIR_FREE(virt_fns[i]); + VIR_FREE(virt_fns); + VIR_FREE(pf_sysfs_device_link); + VIR_FREE(pci_sysfs_device_link); + VIR_FREE(pciConfigAddr); + return ret; +} +
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This element will help the user to just specify the SR-IOV physical function in order to access all the Virtual functions attached to it. --- docs/schemas/network.rng | 7 ++++ src/conf/network_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 3 ++ 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 937e180..64711ba 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -92,6 +92,13 @@ </attribute> </element> </zeroOrMore> + <optional> + <element name='pf'> + <attribute name='dev'> + <ref name='deviceName'/> + </attribute> + </element> + </optional> </element> </optional> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 1058b07..9557e29 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -153,6 +153,11 @@ void virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->bridge); VIR_FREE(def->domain); + for (ii = 0 ; ii < def->nForwardPfs && def->forwardPfs ; ii++) { + virNetworkForwardIfDefClear(&def->forwardPfs[ii]); + } + VIR_FREE(def->forwardPfs); + for (ii = 0 ; ii < def->nForwardIfs && def->forwardIfs ; ii++) { virNetworkForwardIfDefClear(&def->forwardIfs[ii]); } @@ -820,10 +825,11 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) xmlNodePtr *ipNodes = NULL; xmlNodePtr *portGroupNodes = NULL; xmlNodePtr *forwardIfNodes = NULL; + xmlNodePtr *forwardPfNodes = NULL; xmlNodePtr dnsNode = NULL; xmlNodePtr virtPortNode = NULL; xmlNodePtr forwardNode = NULL; - int nIps, nPortGroups, nForwardIfs; + int nIps, nPortGroups, nForwardIfs, nForwardPfs; char *forwardDev = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr bandwidthNode = NULL; @@ -965,10 +971,52 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf")); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given")); goto error; + } + } - if ((nForwardIfs > 0) || forwardDev) { + if (nForwardPfs == 1) { + + if (VIR_ALLOC_N(def->forwardPfs, nForwardPfs) < 0) { + virReportOOMError(); + goto error; + } + + if (forwardDev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("A forward Dev should not be used when using a SRIOV PF")); + goto error; + } + + forwardDev = virXMLPropString(*forwardPfNodes, "dev"); + if (!forwardDev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Missing required dev attribute in network '%s' pf element"), + def->name); + goto error; + } + + def->forwardPfs->usageCount = 0; + def->forwardPfs->dev = forwardDev; + forwardDev = NULL; + def->nForwardPfs++; + } + + else if (nForwardPfs > 1) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Use of more than one physical interface is not allowed")); + goto error; + } + + else if ((nForwardIfs > 0) || forwardDev) { int ii; /* allocate array to hold all the portgroups */ @@ -1014,6 +1062,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->nForwardIfs++; } } + VIR_FREE(forwardDev); + VIR_FREE(forwardPfNodes); VIR_FREE(forwardIfNodes); switch (def->forwardType) { @@ -1069,6 +1119,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) VIR_FREE(ipNodes); VIR_FREE(portGroupNodes); VIR_FREE(forwardIfNodes); + VIR_FREE(forwardPfNodes); VIR_FREE(forwardDev); ctxt->node = save; return NULL; @@ -1290,7 +1341,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", uuidstr); if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { - const char *dev = virNetworkDefForwardIf(def, 0); + char *dev = NULL; + if (def->nForwardPfs < 1) + dev = (char *)virNetworkDefForwardIf(def, 0); const char *mode = virNetworkForwardTypeToString(def->forwardType); if (!mode) { @@ -1305,6 +1358,14 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferAsprintf(&buf, " mode='%s'%s>\n", mode, def->nForwardIfs ? "" : "/"); + + if (def->nForwardPfs) { + if (def->forwardPfs->dev) { + virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + def->forwardPfs->dev); + } + } + if (def->nForwardIfs) { for (ii = 0; ii < def->nForwardIfs; ii++) { if (def->forwardIfs[ii].dev) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1be20f8..e25f8d3 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -146,6 +146,9 @@ struct _virNetworkDef { /* If there are multiple forward devices (i.e. a pool of * interfaces), they will be listed here. */ + size_t nForwardPfs; + virNetworkForwardIfDefPtr forwardPfs; + size_t nForwardIfs; virNetworkForwardIfDefPtr forwardIfs; -- 1.7.4.4
On Wed, Dec 14, 2011 at 10:50:23AM +0000, Shradha Shah wrote:
@@ -965,10 +971,52 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf"));
You don't want to be raising an error at this point, since this could still be a success codepath, and you've got an error in the following failure path anyway.
+ nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given")); goto error;
Small indentation bug
+ } + }
- if ((nForwardIfs > 0) || forwardDev) { + if (nForwardPfs == 1) { + + if (VIR_ALLOC_N(def->forwardPfs, nForwardPfs) < 0) { + virReportOOMError(); + goto error; + } + + if (forwardDev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("A forward Dev should not be used when using a SRIOV PF")); + goto error; + } + + forwardDev = virXMLPropString(*forwardPfNodes, "dev"); + if (!forwardDev) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Missing required dev attribute in network '%s' pf element"), + def->name); + goto error; + } + + def->forwardPfs->usageCount = 0; + def->forwardPfs->dev = forwardDev; + forwardDev = NULL; + def->nForwardPfs++; + } + + else if (nForwardPfs > 1) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Use of more than one physical interface is not allowed")); + goto error; + } + + else if ((nForwardIfs > 0) || forwardDev) { int ii;
/* allocate array to hold all the portgroups */
Same comment as previous patch about 'make syntax-check' to catch any trailing whitespace
@@ -1290,7 +1341,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", uuidstr);
if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { - const char *dev = virNetworkDefForwardIf(def, 0); + char *dev = NULL; + if (def->nForwardPfs < 1) + dev = (char *)virNetworkDefForwardIf(def, 0);
You can avoid casting away const, by rewriting this const char *dev = dev->nForwardPfs ? NULL : virNetworkDefForwardIf(def, 0);
const char *mode = virNetworkForwardTypeToString(def->forwardType);
if (!mode) { @@ -1305,6 +1358,14 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) virBufferAsprintf(&buf, " mode='%s'%s>\n", mode, def->nForwardIfs ? "" : "/");
+ + if (def->nForwardPfs) { + if (def->forwardPfs->dev) {
I'd be slightly happier if this was written as
+ virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + def->forwardPfs->dev); + }
'def->forwardPfs[0].dev' since we have declared this as an array of devs, even if we only allow 1 of them for now.
+ } + if (def->nForwardIfs) { for (ii = 0; ii < def->nForwardIfs; ii++) { if (def->forwardIfs[ii].dev) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1be20f8..e25f8d3 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -146,6 +146,9 @@ struct _virNetworkDef { /* If there are multiple forward devices (i.e. a pool of * interfaces), they will be listed here. */ + size_t nForwardPfs; + virNetworkForwardIfDefPtr forwardPfs; + size_t nForwardIfs; virNetworkForwardIfDefPtr forwardIfs;
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/14/2011 03:50 AM, Shradha Shah wrote:
This element will help the user to just specify the SR-IOV physical function in order to access all the Virtual functions attached to it. --- docs/schemas/network.rng | 7 ++++ src/conf/network_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 3 ++ 3 files changed, 75 insertions(+), 4 deletions(-)
In addition to Daniel's comments,
@@ -965,10 +971,52 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf")); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given"));
This has to be a check for '< 0', not '<= 0', or else you get LOTS of 'make check' failures. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/10/2012 04:33 PM, Eric Blake wrote:
On 12/14/2011 03:50 AM, Shradha Shah wrote:
This element will help the user to just specify the SR-IOV physical function in order to access all the Virtual functions attached to it. --- docs/schemas/network.rng | 7 ++++ src/conf/network_conf.c | 69 +++++++++++++++++++++++++++++++++++++++++++-- src/conf/network_conf.h | 3 ++ 3 files changed, 75 insertions(+), 4 deletions(-)
In addition to Daniel's comments,
@@ -965,10 +971,52 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf")); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given"));
This has to be a check for '< 0', not '<= 0', or else you get LOTS of 'make check' failures.
Also, your patch touched the rng, but failed to add documentation for the new XML, nor tests that prove we can parse it. And in adding those tests, I found that your rng additions don't match your code (which wants pf before, not after, interface). I'm working on fixing that, but it's taking me longer than I planned, since I also decided to add tests of the parsing, and in those tests, it appears that pf did not get parsed as expected. I don't know if it's a flaw in the original patch or in my touchups... -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/10/2012 05:41 PM, Eric Blake wrote:
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf")); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given"));
This has to be a check for '< 0', not '<= 0', or else you get LOTS of 'make check' failures.
Also, your patch touched the rng, but failed to add documentation for the new XML, nor tests that prove we can parse it. And in adding those tests, I found that your rng additions don't match your code (which wants pf before, not after, interface). I'm working on fixing that, but it's taking me longer than I planned, since I also decided to add tests of the parsing, and in those tests, it appears that pf did not get parsed as expected. I don't know if it's a flaw in the original patch or in my touchups...
I think I understand the problem now. You aren't parsing nForwardPfs unless there are exactly 0 nForwardIfs; but if you end up reparsing existing live XML, you don't want to lose the pf information. That is, the parsing routine should always parse pf information; and perhaps even be taught to honor flags (just like the format routine), so that it skips parsing <interface> elements if doing an INACTIVE parse and a pf is present. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/11/2012 11:08 AM, Eric Blake wrote:
On 01/10/2012 05:41 PM, Eric Blake wrote:
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) + if (nForwardIfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool given, checking for SRIOV pf")); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + + if (nForwardPfs <= 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given"));
This has to be a check for '< 0', not '<= 0', or else you get LOTS of 'make check' failures.
Also, your patch touched the rng, but failed to add documentation for the new XML, nor tests that prove we can parse it. And in adding those tests, I found that your rng additions don't match your code (which wants pf before, not after, interface). I'm working on fixing that, but it's taking me longer than I planned, since I also decided to add tests of the parsing, and in those tests, it appears that pf did not get parsed as expected. I don't know if it's a flaw in the original patch or in my touchups...
I think I understand the problem now. You aren't parsing nForwardPfs unless there are exactly 0 nForwardIfs; but if you end up reparsing existing live XML, you don't want to lose the pf information. That is, the parsing routine should always parse pf information; and perhaps even be taught to honor flags (just like the format routine), so that it skips parsing <interface> elements if doing an INACTIVE parse and a pf is present.
I got things to work without a flags on parse, for now, so this is what I'm squashing into 3/5 before pushing: diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 755d510..907c046 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -226,7 +226,21 @@ </forward> ... </pre> - When a guest interface is being constructed, libvirt will pick + Additionally, <span class="since">since 0.9.10</span>, libvirt + allows a shorthand for specifying all virtual interfaces + associated with a single physical function, by using + the <code><pf></code> subelement to call out the + corresponding physical interface associated with multiple + virtual interfaces: + <pre> +... + <forward mode='passthrough'> + <pf dev='eth0'/> + </forward> +... + </pre> + + <p>When a guest interface is being constructed, libvirt will pick an interface from this list to use for the connection. In modes where physical interfaces can be shared by multiple guest interfaces, libvirt will choose the interface that @@ -234,7 +248,7 @@ that do not allow sharing of the physical device (in particular, 'passthrough' mode, and 'private' mode when using 802.1Qbh), libvirt will choose an unused physical interface - or, if it can't find an unused interface, fail the operation. + or, if it can't find an unused interface, fail the operation.</p> </dd> </dl> <h5><a name="elementQoS">Quality of service</a></h5> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 9a5d575..6e1002f 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -85,20 +85,22 @@ </choice> </attribute> </optional> - <zeroOrMore> - <element name='interface'> - <attribute name='dev'> - <ref name='deviceName'/> - </attribute> - </element> - </zeroOrMore> - <optional> - <element name='pf'> - <attribute name='dev'> - <ref name='deviceName'/> - </attribute> - </element> - </optional> + <interleave> + <zeroOrMore> + <element name='interface'> + <attribute name='dev'> + <ref name='deviceName'/> + </attribute> + </element> + </zeroOrMore> + <optional> + <element name='pf'> + <attribute name='dev'> + <ref name='deviceName'/> + </attribute> + </element> + </optional> + </interleave> </element> </optional> diff --git a/tests/networkxml2xmlin/passthrough-pf.xml b/tests/networkxml2xmlin/passthrough-pf.xml new file mode 100644 index 0000000..e63aae0 --- /dev/null +++ b/tests/networkxml2xmlin/passthrough-pf.xml @@ -0,0 +1,10 @@ +<network> + <name>local</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode="passthrough"> + <pf dev='eth0'/> + <interface dev='eth10'/> + <interface dev='eth11'/> + </forward> + <ip address="192.168.122.1" netmask="255.255.255.0"/> +</network> diff --git a/tests/networkxml2xmlout/passthrough-pf.xml b/tests/networkxml2xmlout/passthrough-pf.xml new file mode 100644 index 0000000..1a7e71e --- /dev/null +++ b/tests/networkxml2xmlout/passthrough-pf.xml @@ -0,0 +1,9 @@ +<network> + <name>local</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode='passthrough'> + <pf dev='eth0'/> + </forward> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> -- 1.7.7.5 -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If a system has 64 or more VF's, it is quite tedious to mention each VF in the interface pool. The following modification will implicitly create an interface pool from the SR-IOV PF. --- src/network/bridge_driver.c | 93 +++++++++++++++++++++++++++++++++--------- 1 files changed, 73 insertions(+), 20 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 63338a2..9ae9c50 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2817,15 +2817,19 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) /* If there is only a single device, just return it (caller will detect * any error if exclusive use is required but could not be acquired). */ - if (netdef->nForwardIfs == 0) { + if ((netdef->nForwardIfs <= 0) && (netdef->nForwardPfs <= 0)) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + + else { int ii; virNetworkForwardIfDefPtr dev = NULL; - + unsigned int num_virt_fns = 0; + char **vfname = NULL; + /* pick an interface from the pool */ /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require @@ -2833,20 +2837,67 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) * 0. Other modes can share, so just search for the one with * the lowest usageCount. */ - if ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || - ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && - iface->data.network.actual->data.direct.virtPortProfile && - (iface->data.network.actual->data.direct.virtPortProfile->virtPortType - == VIR_NETDEV_VPORT_PROFILE_8021QBH))) { + if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) { + if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) { + if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, + &vfname, &num_virt_fns)) < 0){ + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get Virtual functions on %s"), + netdef->forwardPfs->dev); + goto out; + } + + if (num_virt_fns == 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("No Vf's present on SRIOV PF %s"), + netdef->forwardPfs->dev); + goto out; + } + + if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { + virReportOOMError(); + goto out; + } + + netdef->nForwardIfs = num_virt_fns; + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + netdef->forwardIfs[ii].dev = strdup(vfname[ii]); + netdef->forwardIfs[ii].usageCount = 0; + } + + out: + for (ii = 0; ii < num_virt_fns; ii++) { + VIR_FREE(vfname[ii]); + } + VIR_FREE(vfname); + } + /* pick first dev with 0 usageCount */ - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].usageCount == 0) { dev = &netdef->forwardIfs[ii]; break; } } - } else { + } + else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + iface->data.network.actual->data.direct.virtPortProfile && + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_NETDEV_VPORT_PROFILE_8021QBH)) { + + + /* pick first dev with 0 usageCount */ + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].usageCount == 0) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + } + else { /* pick least used dev */ dev = &netdef->forwardIfs[0]; for (ii = 1; ii < netdef->nForwardIfs; ii++) { @@ -2858,7 +2909,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) if (!dev) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' requires exclusive access to interfaces, but none are available"), - netdef->name); + netdef->name); goto cleanup; } iface->data.network.actual->data.direct.linkdev = strdup(dev->dev); @@ -2901,6 +2952,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; const char *actualDev; + virNetworkForwardIfDefPtr dev = NULL; int ret = -1; if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) @@ -2935,12 +2987,12 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + else { int ii; - virNetworkForwardIfDefPtr dev = NULL; - + /* find the matching interface in the pool and increment its usageCount */ - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { dev = &netdef->forwardIfs[ii]; @@ -2954,7 +3006,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) netdef->name, actualDev); goto cleanup; } - + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require * exclusive access to a device, so current usageCount must be * 0 in those cases. @@ -3001,6 +3053,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network = NULL; virNetworkDefPtr netdef; const char *actualDev; + virNetworkForwardIfDefPtr dev = NULL; int ret = -1; if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) @@ -3036,10 +3089,10 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + else { int ii; - virNetworkForwardIfDefPtr dev = NULL; - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { dev = &netdef->forwardIfs[ii]; @@ -3053,7 +3106,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) netdef->name, actualDev); goto cleanup; } - + dev->usageCount--; VIR_DEBUG("Releasing physical device %s, usageCount %d", dev->dev, dev->usageCount); -- 1.7.4.4
On Wed, Dec 14, 2011 at 10:50:30AM +0000, Shradha Shah wrote:
If a system has 64 or more VF's, it is quite tedious to mention each VF in the interface pool. The following modification will implicitly create an interface pool from the SR-IOV PF. --- src/network/bridge_driver.c | 93 +++++++++++++++++++++++++++++++++--------- 1 files changed, 73 insertions(+), 20 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 63338a2..9ae9c50 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2817,15 +2817,19 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) /* If there is only a single device, just return it (caller will detect * any error if exclusive use is required but could not be acquired). */ - if (netdef->nForwardIfs == 0) { + if ((netdef->nForwardIfs <= 0) && (netdef->nForwardPfs <= 0)) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + + else {
Minor style issue - can you keep the '} else {' alignment.
int ii; virNetworkForwardIfDefPtr dev = NULL; - + unsigned int num_virt_fns = 0; + char **vfname = NULL; + /* pick an interface from the pool */
/* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require @@ -2833,20 +2837,67 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) * 0. Other modes can share, so just search for the one with * the lowest usageCount. */ - if ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) || - ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && - iface->data.network.actual->data.direct.virtPortProfile && - (iface->data.network.actual->data.direct.virtPortProfile->virtPortType - == VIR_NETDEV_VPORT_PROFILE_8021QBH))) { + if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) { + if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) { + if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, + &vfname, &num_virt_fns)) < 0){ + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get Virtual functions on %s"), + netdef->forwardPfs->dev); + goto out; + } + + if (num_virt_fns == 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("No Vf's present on SRIOV PF %s"), + netdef->forwardPfs->dev); + goto out; + } + + if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) { + virReportOOMError(); + goto out; + } + + netdef->nForwardIfs = num_virt_fns; + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + netdef->forwardIfs[ii].dev = strdup(vfname[ii]);
Missing check for OOM here
+ netdef->forwardIfs[ii].usageCount = 0; + } + + out: + for (ii = 0; ii < num_virt_fns; ii++) { + VIR_FREE(vfname[ii]); + } + VIR_FREE(vfname); + } + /* pick first dev with 0 usageCount */ - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (netdef->forwardIfs[ii].usageCount == 0) { dev = &netdef->forwardIfs[ii]; break; } } - } else { + } + else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) && + iface->data.network.actual->data.direct.virtPortProfile && + (iface->data.network.actual->data.direct.virtPortProfile->virtPortType + == VIR_NETDEV_VPORT_PROFILE_8021QBH)) {
Also '} else if '
+ + + /* pick first dev with 0 usageCount */ + + for (ii = 0; ii < netdef->nForwardIfs; ii++) { + if (netdef->forwardIfs[ii].usageCount == 0) { + dev = &netdef->forwardIfs[ii]; + break; + } + } + } + else {
And '} else {'
/* pick least used dev */ dev = &netdef->forwardIfs[0]; for (ii = 1; ii < netdef->nForwardIfs; ii++) { @@ -2858,7 +2909,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) if (!dev) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' requires exclusive access to interfaces, but none are available"), - netdef->name); + netdef->name); goto cleanup; } iface->data.network.actual->data.direct.linkdev = strdup(dev->dev);
From this point onwards, AFAICT, none of the patch hunks have any functional change. They're all just whitespace changes, or moving variable declarations. So I think the rest of this patch can just be dropped.
@@ -2901,6 +2952,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network; virNetworkDefPtr netdef; const char *actualDev; + virNetworkForwardIfDefPtr dev = NULL; int ret = -1;
if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) @@ -2935,12 +2987,12 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + else {
Change back to '} else {'
int ii; - virNetworkForwardIfDefPtr dev = NULL; - + /* find the matching interface in the pool and increment its usageCount */ - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { dev = &netdef->forwardIfs[ii]; @@ -2954,7 +3006,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) netdef->name, actualDev); goto cleanup; } - + /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require * exclusive access to a device, so current usageCount must be * 0 in those cases. @@ -3001,6 +3053,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) virNetworkObjPtr network = NULL; virNetworkDefPtr netdef; const char *actualDev; + virNetworkForwardIfDefPtr dev = NULL; int ret = -1;
if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) @@ -3036,10 +3089,10 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) _("network '%s' uses a direct mode, but has no forward dev and no interface pool"), netdef->name); goto cleanup; - } else { + } + else {
And here
int ii; - virNetworkForwardIfDefPtr dev = NULL; - + for (ii = 0; ii < netdef->nForwardIfs; ii++) { if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) { dev = &netdef->forwardIfs[ii]; @@ -3053,7 +3106,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) netdef->name, actualDev); goto cleanup; } - + dev->usageCount--; VIR_DEBUG("Releasing physical device %s, usageCount %d", dev->dev, dev->usageCount);
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The above option helps to differentiate between implicit and explicit interface pools. --- include/libvirt/libvirt.h.in | 4 ++++ src/conf/network_conf.c | 10 +++++++--- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 4 ++-- src/test/test_driver.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++++++++-- 8 files changed, 28 insertions(+), 11 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..45fe060 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1783,6 +1783,10 @@ int virNodeGetCellsFreeMemory(virConnectPtr conn, * Virtual Networks API */ +typedef enum { + VIR_NETWORK_XML_INACTIVE = (1 << 0), /* dump inactive network information */ +} virNetworkXMLFlags; + /** * virNetwork: * diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9557e29..37b7454 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1326,7 +1326,7 @@ virPortGroupDefFormat(virBufferPtr buf, return 0; } -char *virNetworkDefFormat(const virNetworkDefPtr def) +char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned char *uuid; @@ -1366,6 +1366,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) } } + if (flags && def->nForwardPfs) + goto escape; + if (def->nForwardIfs) { for (ii = 0; ii < def->nForwardIfs; ii++) { if (def->forwardIfs[ii].dev) { @@ -1373,8 +1376,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) def->forwardIfs[ii].dev); } } - virBufferAddLit(&buf, " </forward>\n"); } + escape: + virBufferAddLit(&buf, " </forward>\n"); } if (def->forwardType == VIR_NETWORK_FORWARD_NONE || @@ -1484,7 +1488,7 @@ int virNetworkSaveConfig(const char *configDir, int ret = -1; char *xml; - if (!(xml = virNetworkDefFormat(def))) + if (!(xml = virNetworkDefFormat(def, 0))) goto cleanup; if (virNetworkSaveXML(configDir, def, xml)) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e25f8d3..27e568b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -211,7 +211,7 @@ virNetworkDefPtr virNetworkDefParseFile(const char *filename); virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml, xmlNodePtr root); -char *virNetworkDefFormat(const virNetworkDefPtr def); +char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags); static inline const char * virNetworkDefForwardIf(const virNetworkDefPtr def, size_t n) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9ae9c50..feacd4a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2482,7 +2482,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, virNetworkObjPtr network; char *ret = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL); networkDriverLock(driver); network = virNetworkFindByUUID(&driver->networks, net->uuid); @@ -2494,7 +2494,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, goto cleanup; } - ret = virNetworkDefFormat(network->def); + ret = virNetworkDefFormat(network->def, flags); cleanup: if (network) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89f7df1..b88bdfe 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3264,7 +3264,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, goto cleanup; } - ret = virNetworkDefFormat(privnet->def); + ret = virNetworkDefFormat(privnet->def, flags); cleanup: if (privnet) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9b74a7b..270f106 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8132,7 +8132,7 @@ static char *vboxNetworkGetXMLDesc(virNetworkPtr network, VBOX_UTF16_FREE(networkInterfaceNameUtf16); VBOX_RELEASE(host); - ret = virNetworkDefFormat(def); + ret = virNetworkDefFormat(def, 0); cleanup: virNetworkDefFree(def); diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 5cdbedb..4249caa 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -30,7 +30,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; - if (!(actual = virNetworkDefFormat(dev))) + if (!(actual = virNetworkDefFormat(dev, 0))) goto fail; if (STRNEQ(outXmlData, actual)) { diff --git a/tools/virsh.c b/tools/virsh.c index 90ff587..87cf48b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6619,6 +6619,7 @@ static const vshCmdInfo info_network_dumpxml[] = { static const vshCmdOptDef opts_network_dumpxml[] = { {"network", VSH_OT_DATA, VSH_OFLAG_REQ, N_("network name or uuid")}, + {"inactive", VSH_OT_BOOL, VSH_OFLAG_NONE, N_("network information of an inactive domain")}, {NULL, 0, 0, NULL} }; @@ -6628,14 +6629,22 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) virNetworkPtr network; bool ret = true; char *dump; - + unsigned int flags = 0; + int inactive; + if (!vshConnectionUsability(ctl, ctl->conn)) return false; if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; + + inactive = vshCommandOptBool (cmd, "inactive"); + + if (inactive) + flags |= VIR_NETWORK_XML_INACTIVE; + + dump = virNetworkGetXMLDesc(network, flags); - dump = virNetworkGetXMLDesc(network, 0); if (dump != NULL) { vshPrint(ctl, "%s", dump); VIR_FREE(dump); -- 1.7.4.4
On Wed, Dec 14, 2011 at 10:50:40AM +0000, Shradha Shah wrote:
The above option helps to differentiate between implicit and explicit interface pools. --- include/libvirt/libvirt.h.in | 4 ++++ src/conf/network_conf.c | 10 +++++++--- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 4 ++-- src/test/test_driver.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++++++++-- 8 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..45fe060 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1783,6 +1783,10 @@ int virNodeGetCellsFreeMemory(virConnectPtr conn, * Virtual Networks API */
+typedef enum { + VIR_NETWORK_XML_INACTIVE = (1 << 0), /* dump inactive network information */ +} virNetworkXMLFlags; + /** * virNetwork: * diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9557e29..37b7454 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1326,7 +1326,7 @@ virPortGroupDefFormat(virBufferPtr buf, return 0; }
-char *virNetworkDefFormat(const virNetworkDefPtr def) +char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned char *uuid; @@ -1366,6 +1366,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) } }
+ if (flags && def->nForwardPfs) + goto escape;
You'll want to be explicit about what you're checking here '(flags & VIR_NETWORK_XML_INACTIVE) && def->nForwardPfs' so this code doesn't break if we add more flags in the future
+ if (def->nForwardIfs) {
And rather than having a goto above, I'd just change this to if (def->nForwardIfs && !(flags & VIR_NETWORK_XML_INACTIVE) && def->nForwardPfs) {
for (ii = 0; ii < def->nForwardIfs; ii++) { if (def->forwardIfs[ii].dev) { @@ -1373,8 +1376,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) def->forwardIfs[ii].dev); } } - virBufferAddLit(&buf, " </forward>\n"); } + escape: + virBufferAddLit(&buf, " </forward>\n"); }
if (def->forwardType == VIR_NETWORK_FORWARD_NONE ||
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/14/2011 03:50 AM, Shradha Shah wrote:
The above option helps to differentiate between implicit and explicit interface pools. --- include/libvirt/libvirt.h.in | 4 ++++ src/conf/network_conf.c | 10 +++++++--- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 4 ++-- src/test/test_driver.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++++++++-- 8 files changed, 28 insertions(+), 11 deletions(-)
In addition to Daniel's comments, you are missing documentation for the new flag; src/libvirt.c needs to mention the valid 'flags' value, and tools/virsh.pod needs to document the new code. I'm squashing this to your last patch, then rebasing things slightly (I'm moving portions of network_conf.c that format pf on output to patch 3, to parallel parsing it on input, so that this patch is only left with adding flags support to network_conf.c), then pushing the series. diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 21b2d0a..009479c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1080,14 +1080,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs <= 0) { - nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); - if (nForwardPfs < 0) { - virNetworkReportError(VIR_ERR_XML_ERROR, - _("No interface pool or SRIOV physical device given")); - goto error; - } + if (nForwardIfs < 0 || nForwardPfs < 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given")); + goto error; } if (nForwardPfs == 1) { @@ -1118,7 +1116,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) virNetworkReportError(VIR_ERR_XML_ERROR, _("Use of more than one physical interface is not allowed")); goto error; - } else if ((nForwardIfs > 0) || forwardDev) { + } + if (nForwardIfs > 0 || forwardDev) { int ii; /* allocate array to hold all the portgroups */ @@ -1465,7 +1464,7 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { const char *dev = NULL; - if (def->nForwardPfs < 1) + if (!def->nForwardPfs) dev = virNetworkDefForwardIf(def, 0); const char *mode = virNetworkForwardTypeToString(def->forwardType); @@ -1476,31 +1475,24 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) goto error; } virBufferAddLit(&buf, " <forward"); - if (dev) - virBufferEscapeString(&buf, " dev='%s'", dev); + virBufferEscapeString(&buf, " dev='%s'", dev); virBufferAsprintf(&buf, " mode='%s'%s>\n", mode, - def->nForwardIfs ? "" : "/"); + (def->nForwardIfs || def->nForwardPfs) ? "" : "/"); - if (def->nForwardPfs) { - if (def->forwardPfs->dev) { - virBufferEscapeString(&buf, " <pf dev='%s'/>\n", - def->forwardPfs[0].dev); - } - } + /* For now, hard-coded to at most 1 forwardPfs */ + if (def->forwardPfs) + virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + def->forwardPfs[0].dev); - if (flags && def->nForwardPfs) - goto escape; - - if (def->nForwardIfs) { + if (def->nForwardIfs && + (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) { for (ii = 0; ii < def->nForwardIfs; ii++) { - if (def->forwardIfs[ii].dev) { - virBufferEscapeString(&buf, " <interface dev='%s'/>\n", - def->forwardIfs[ii].dev); - } + virBufferEscapeString(&buf, " <interface dev='%s'/>\n", + def->forwardIfs[ii].dev); } } - escape: - virBufferAddLit(&buf, " </forward>\n"); + if (def->nForwardPfs || def->nForwardIfs) + virBufferAddLit(&buf, " </forward>\n"); } if (def->forwardType == VIR_NETWORK_FORWARD_NONE || diff --git a/src/libvirt.c b/src/libvirt.c index dbf0296..a540424 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9835,11 +9835,16 @@ error: /** * virNetworkGetXMLDesc: * @network: a network object - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virNetworkXMLFlags * * Provide an XML description of the network. The description may be reused * later to relaunch the network with virNetworkCreateXML(). * + * Normally, if a network included a physical function, the output includes + * all virtual functions tied to that physical interface. If @flags includes + * VIR_NETWORK_XML_INACTIVE, then the expansion of virtual interfaces is + * not performed. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b5c0b8..2a98e7e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1,7 +1,7 @@ /* * test.c: A "mock" hypervisor for use by application unit tests * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 4249caa..6cb9d90 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -14,7 +14,8 @@ #include "testutilsqemu.h" static int -testCompareXMLToXMLFiles(const char *inxml, const char *outxml) +testCompareXMLToXMLFiles(const char *inxml, const char *outxml, + unsigned int flags) { char *inXmlData = NULL; char *outXmlData = NULL; @@ -30,7 +31,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; - if (!(actual = virNetworkDefFormat(dev, 0))) + if (!(actual = virNetworkDefFormat(dev, flags))) goto fail; if (STRNEQ(outXmlData, actual)) { @@ -48,21 +49,27 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) return ret; } +struct testInfo { + const char *name; + unsigned int flags; +}; + static int testCompareXMLToXMLHelper(const void *data) { + const struct testInfo *info = data; int result = -1; char *inxml = NULL; char *outxml = NULL; if (virAsprintf(&inxml, "%s/networkxml2xmlin/%s.xml", - abs_srcdir, (const char*)data) < 0 || + abs_srcdir, info->name) < 0 || virAsprintf(&outxml, "%s/networkxml2xmlout/%s.xml", - abs_srcdir, (const char*)data) < 0) { + abs_srcdir, info->name) < 0) { goto cleanup; } - result = testCompareXMLToXMLFiles(inxml, outxml); + result = testCompareXMLToXMLFiles(inxml, outxml, info->flags); cleanup: free(inxml); @@ -76,10 +83,14 @@ mymain(void) { int ret = 0; -#define DO_TEST(name) \ - if (virtTestRun("Network XML-2-XML " name, \ - 1, testCompareXMLToXMLHelper, (name)) < 0) \ - ret = -1 +#define DO_TEST_FULL(name, flags) \ + do { \ + const struct testInfo info = {name, flags}; \ + if (virtTestRun("Network XML-2-XML " name, \ + 1, testCompareXMLToXMLHelper, &info) < 0) \ + ret = -1; \ + } while (0) +#define DO_TEST(name) DO_TEST_FULL(name, 0) DO_TEST("isolated-network"); DO_TEST("routed-network"); @@ -93,6 +104,7 @@ mymain(void) DO_TEST("host-bridge-net"); DO_TEST("vepa-net"); DO_TEST("bandwidth-network"); + DO_TEST_FULL("passthrough-pf", VIR_NETWORK_XML_INACTIVE); return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } diff --git a/tools/virsh.c b/tools/virsh.c index ba07e0f..3869d9d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7298,15 +7298,14 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) char *dump; unsigned int flags = 0; int inactive; - + if (!vshConnectionUsability(ctl, ctl->conn)) return false; if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; - + inactive = vshCommandOptBool (cmd, "inactive"); - if (inactive) flags |= VIR_NETWORK_XML_INACTIVE; diff --git a/tools/virsh.pod b/tools/virsh.pod index 1abf448..f2793cd 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1523,9 +1523,11 @@ not instantiated. Destroy (stop) a given virtual network specified by its name or UUID. This takes effect immediately. -=item B<net-dumpxml> I<network> +=item B<net-dumpxml> I<network> [I<--inactive>] Output the virtual network information as an XML dump to stdout. +If I<--inactive> is specified, then physical functions are not +expanded into their associated virtual functions. =item B<net-edit> I<network> -- 1.7.7.5 -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric, Many Thanks for your comments and for pushing the patches through. I understand the review comments you and Daniel posted on my patch series and was going to work on them today. I saw your modified patches and wanted to thank you for your contribution towards the patch series and for pushing them upstream. Regards, Shradha Shah On 01/11/2012 07:31 PM, Eric Blake wrote:
On 12/14/2011 03:50 AM, Shradha Shah wrote:
The above option helps to differentiate between implicit and explicit interface pools. --- include/libvirt/libvirt.h.in | 4 ++++ src/conf/network_conf.c | 10 +++++++--- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 4 ++-- src/test/test_driver.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++++++++-- 8 files changed, 28 insertions(+), 11 deletions(-)
In addition to Daniel's comments, you are missing documentation for the new flag; src/libvirt.c needs to mention the valid 'flags' value, and tools/virsh.pod needs to document the new code.
I'm squashing this to your last patch, then rebasing things slightly (I'm moving portions of network_conf.c that format pf on output to patch 3, to parallel parsing it on input, so that this patch is only left with adding flags support to network_conf.c), then pushing the series.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 21b2d0a..009479c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1080,14 +1080,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
/* all of these modes can use a pool of physical interfaces */ nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs <= 0) { - nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); + nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes);
- if (nForwardPfs < 0) { - virNetworkReportError(VIR_ERR_XML_ERROR, - _("No interface pool or SRIOV physical device given")); - goto error; - } + if (nForwardIfs < 0 || nForwardPfs < 0) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("No interface pool or SRIOV physical device given")); + goto error; }
if (nForwardPfs == 1) { @@ -1118,7 +1116,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) virNetworkReportError(VIR_ERR_XML_ERROR, _("Use of more than one physical interface is not allowed")); goto error; - } else if ((nForwardIfs > 0) || forwardDev) { + } + if (nForwardIfs > 0 || forwardDev) { int ii;
/* allocate array to hold all the portgroups */ @@ -1465,7 +1464,7 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { const char *dev = NULL; - if (def->nForwardPfs < 1) + if (!def->nForwardPfs) dev = virNetworkDefForwardIf(def, 0); const char *mode = virNetworkForwardTypeToString(def->forwardType);
@@ -1476,31 +1475,24 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) goto error; } virBufferAddLit(&buf, " <forward"); - if (dev) - virBufferEscapeString(&buf, " dev='%s'", dev); + virBufferEscapeString(&buf, " dev='%s'", dev); virBufferAsprintf(&buf, " mode='%s'%s>\n", mode, - def->nForwardIfs ? "" : "/"); + (def->nForwardIfs || def->nForwardPfs) ? "" : "/");
- if (def->nForwardPfs) { - if (def->forwardPfs->dev) { - virBufferEscapeString(&buf, " <pf dev='%s'/>\n", - def->forwardPfs[0].dev); - } - } + /* For now, hard-coded to at most 1 forwardPfs */ + if (def->forwardPfs) + virBufferEscapeString(&buf, " <pf dev='%s'/>\n", + def->forwardPfs[0].dev);
- if (flags && def->nForwardPfs) - goto escape; - - if (def->nForwardIfs) { + if (def->nForwardIfs && + (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) { for (ii = 0; ii < def->nForwardIfs; ii++) { - if (def->forwardIfs[ii].dev) { - virBufferEscapeString(&buf, " <interface dev='%s'/>\n", - def->forwardIfs[ii].dev); - } + virBufferEscapeString(&buf, " <interface dev='%s'/>\n", + def->forwardIfs[ii].dev); } } - escape: - virBufferAddLit(&buf, " </forward>\n"); + if (def->nForwardPfs || def->nForwardIfs) + virBufferAddLit(&buf, " </forward>\n"); }
if (def->forwardType == VIR_NETWORK_FORWARD_NONE || diff --git a/src/libvirt.c b/src/libvirt.c index dbf0296..a540424 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9835,11 +9835,16 @@ error: /** * virNetworkGetXMLDesc: * @network: a network object - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virNetworkXMLFlags * * Provide an XML description of the network. The description may be reused * later to relaunch the network with virNetworkCreateXML(). * + * Normally, if a network included a physical function, the output includes + * all virtual functions tied to that physical interface. If @flags includes + * VIR_NETWORK_XML_INACTIVE, then the expansion of virtual interfaces is + * not performed. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1b5c0b8..2a98e7e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1,7 +1,7 @@ /* * test.c: A "mock" hypervisor for use by application unit tests * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 4249caa..6cb9d90 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -14,7 +14,8 @@ #include "testutilsqemu.h"
static int -testCompareXMLToXMLFiles(const char *inxml, const char *outxml) +testCompareXMLToXMLFiles(const char *inxml, const char *outxml, + unsigned int flags) { char *inXmlData = NULL; char *outXmlData = NULL; @@ -30,7 +31,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) if (!(dev = virNetworkDefParseString(inXmlData))) goto fail;
- if (!(actual = virNetworkDefFormat(dev, 0))) + if (!(actual = virNetworkDefFormat(dev, flags))) goto fail;
if (STRNEQ(outXmlData, actual)) { @@ -48,21 +49,27 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) return ret; }
+struct testInfo { + const char *name; + unsigned int flags; +}; + static int testCompareXMLToXMLHelper(const void *data) { + const struct testInfo *info = data; int result = -1; char *inxml = NULL; char *outxml = NULL;
if (virAsprintf(&inxml, "%s/networkxml2xmlin/%s.xml", - abs_srcdir, (const char*)data) < 0 || + abs_srcdir, info->name) < 0 || virAsprintf(&outxml, "%s/networkxml2xmlout/%s.xml", - abs_srcdir, (const char*)data) < 0) { + abs_srcdir, info->name) < 0) { goto cleanup; }
- result = testCompareXMLToXMLFiles(inxml, outxml); + result = testCompareXMLToXMLFiles(inxml, outxml, info->flags);
cleanup: free(inxml); @@ -76,10 +83,14 @@ mymain(void) { int ret = 0;
-#define DO_TEST(name) \ - if (virtTestRun("Network XML-2-XML " name, \ - 1, testCompareXMLToXMLHelper, (name)) < 0) \ - ret = -1 +#define DO_TEST_FULL(name, flags) \ + do { \ + const struct testInfo info = {name, flags}; \ + if (virtTestRun("Network XML-2-XML " name, \ + 1, testCompareXMLToXMLHelper, &info) < 0) \ + ret = -1; \ + } while (0) +#define DO_TEST(name) DO_TEST_FULL(name, 0)
DO_TEST("isolated-network"); DO_TEST("routed-network"); @@ -93,6 +104,7 @@ mymain(void) DO_TEST("host-bridge-net"); DO_TEST("vepa-net"); DO_TEST("bandwidth-network"); + DO_TEST_FULL("passthrough-pf", VIR_NETWORK_XML_INACTIVE);
return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } diff --git a/tools/virsh.c b/tools/virsh.c index ba07e0f..3869d9d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7298,15 +7298,14 @@ cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) char *dump; unsigned int flags = 0; int inactive; - + if (!vshConnectionUsability(ctl, ctl->conn)) return false;
if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) return false; - + inactive = vshCommandOptBool (cmd, "inactive"); - if (inactive) flags |= VIR_NETWORK_XML_INACTIVE;
diff --git a/tools/virsh.pod b/tools/virsh.pod index 1abf448..f2793cd 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1523,9 +1523,11 @@ not instantiated. Destroy (stop) a given virtual network specified by its name or UUID. This takes effect immediately.
-=item B<net-dumpxml> I<network> +=item B<net-dumpxml> I<network> [I<--inactive>]
Output the virtual network information as an XML dump to stdout. +If I<--inactive> is specified, then physical functions are not +expanded into their associated virtual functions.
=item B<net-edit> I<network>
Hello All, I had posted this latest version of my patch series a couple of weeks ago. Since I did not receive any feedback I think this post got missed amidst the volume of mail on libvir-list. Since I am new to libvirt I would be happy to receive review comments and advice on my patches. Many thanks, Regards, Shradha Shah On 12/14/2011 10:47 AM, Shradha Shah wrote:
Interface Pools and Passthrough mode:
Current Method: The passthrough mode uses a macvtap direct connection to connect each guest to the network. The physical interface to be used is picked from among those listed in <interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more <interface> sub-elements: Example: <forward mode='passthrough' dev='eth10'/> <interface dev='eth10'/> <interface dev='eth12'/> <interface dev='eth18'/> <interface dev='eth20'/> </forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are multiple non-contiguous ranges.
Proposed Method: The 4 patches provided:
i) Introduce a new element 'pf' to implicitly create an interface pool of all the Virtual Functions attached to the specified Physical Function. ii) Modify the networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice API to use the above mentioned interface pool in the passthrough mode. iii) Allow virsh net-dumpxml to use an option --inactive to differentiate between explicit and implicit interface pools
Hence Libvirt will now support both the methods mentioned below:
* Explicit interface list. App inputs:
<forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'> <pf dev='eth0'/> </forward>
libvirt expands XML to be
<forward mode='passthrough'> <pf dev='eth0'/> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
In the above case we need to differentiate between the implicit and explicit interface pool, which can be done by comparing the dumpxml from active and inactive domains. This will need the addition of the flag VIR_NETWORK_XML_INACTIVE to virNetworkGetXMLDesc().
This patch series supports the use of option --inactive with virsh net-dumpxml.
Shradha Shah (5): Added function pciSysfsFile to enable access to the PCI SYSFS files. Added Function virNetDevGetVirtualFunctions Adding the element pf to network xml. Functionality to implicitly get interface pool from SR-IOV PF. Added new option to virsh net-dumpxml called --inactive
docs/schemas/network.rng | 7 +++ include/libvirt/libvirt.h.in | 4 ++ src/conf/network_conf.c | 79 +++++++++++++++++++++++++++++++--- src/conf/network_conf.h | 5 ++- src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++--------- src/test/test_driver.c | 2 +- src/util/pci.c | 39 +++++++++++++++++ src/util/pci.h | 7 +++ src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++- 13 files changed, 311 insertions(+), 35 deletions(-)
Hello All, I had posted this latest version of my patch series a month ago. Since I did not receive any feedback I think this post got missed amidst the volume of mail on libvir-list. Since I am new to libvirt I would be happy to receive review comments and advice on my patches. Many thanks, Regards, Shradha Shah On 12/14/2011 10:47 AM, Shradha Shah wrote:
Interface Pools and Passthrough mode:
Current Method: The passthrough mode uses a macvtap direct connection to connect each guest to the network. The physical interface to be used is picked from among those listed in <interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more <interface> sub-elements: Example: <forward mode='passthrough' dev='eth10'/> <interface dev='eth10'/> <interface dev='eth12'/> <interface dev='eth18'/> <interface dev='eth20'/> </forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are multiple non-contiguous ranges.
Proposed Method: The 4 patches provided:
i) Introduce a new element 'pf' to implicitly create an interface pool of all the Virtual Functions attached to the specified Physical Function. ii) Modify the networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice API to use the above mentioned interface pool in the passthrough mode. iii) Allow virsh net-dumpxml to use an option --inactive to differentiate between explicit and implicit interface pools
Hence Libvirt will now support both the methods mentioned below:
* Explicit interface list. App inputs:
<forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'> <pf dev='eth0'/> </forward>
libvirt expands XML to be
<forward mode='passthrough'> <pf dev='eth0'/> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
In the above case we need to differentiate between the implicit and explicit interface pool, which can be done by comparing the dumpxml from active and inactive domains. This will need the addition of the flag VIR_NETWORK_XML_INACTIVE to virNetworkGetXMLDesc().
This patch series supports the use of option --inactive with virsh net-dumpxml.
Shradha Shah (5): Added function pciSysfsFile to enable access to the PCI SYSFS files. Added Function virNetDevGetVirtualFunctions Adding the element pf to network xml. Functionality to implicitly get interface pool from SR-IOV PF. Added new option to virsh net-dumpxml called --inactive
docs/schemas/network.rng | 7 +++ include/libvirt/libvirt.h.in | 4 ++ src/conf/network_conf.c | 79 +++++++++++++++++++++++++++++++--- src/conf/network_conf.h | 5 ++- src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++--------- src/test/test_driver.c | 2 +- src/util/pci.c | 39 +++++++++++++++++ src/util/pci.h | 7 +++ src/util/virnetdev.c | 83 ++++++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 6 +++ src/vbox/vbox_tmpl.c | 2 +- tests/networkxml2xmltest.c | 2 +- tools/virsh.c | 13 +++++- 13 files changed, 311 insertions(+), 35 deletions(-)
On Wed, Dec 14, 2011 at 10:47:23AM +0000, Shradha Shah wrote:
Interface Pools and Passthrough mode:
Current Method: The passthrough mode uses a macvtap direct connection to connect each guest to the network. The physical interface to be used is picked from among those listed in <interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more <interface> sub-elements: Example: <forward mode='passthrough' dev='eth10'/> <interface dev='eth10'/> <interface dev='eth12'/> <interface dev='eth18'/> <interface dev='eth20'/> </forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are multiple non-contiguous ranges.
Proposed Method: The 4 patches provided:
i) Introduce a new element 'pf' to implicitly create an interface pool of all the Virtual Functions attached to the specified Physical Function. ii) Modify the networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice API to use the above mentioned interface pool in the passthrough mode. iii) Allow virsh net-dumpxml to use an option --inactive to differentiate between explicit and implicit interface pools
Hence Libvirt will now support both the methods mentioned below:
* Explicit interface list. App inputs:
<forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'> <pf dev='eth0'/> </forward>
libvirt expands XML to be
<forward mode='passthrough'> <pf dev='eth0'/> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
In the above case we need to differentiate between the implicit and explicit interface pool, which can be done by comparing the dumpxml from active and inactive domains. This will need the addition of the flag VIR_NETWORK_XML_INACTIVE to virNetworkGetXMLDesc().
This patch series supports the use of option --inactive with virsh net-dumpxml.
The design and code all looks good to me. Just some fairly minor code & style bugs to deal with, which I've replied to inline. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/10/2012 11:23 AM, Daniel P. Berrange wrote:
On Wed, Dec 14, 2011 at 10:47:23AM +0000, Shradha Shah wrote:
Interface Pools and Passthrough mode:
Current Method: The passthrough mode uses a macvtap direct connection to connect each guest to the network. The physical interface to be used is picked from among those listed in <interface> sub elements of the <forward> element.
The current specification for <forward> extends to allow 0 or more <interface> sub-elements: Example: <forward mode='passthrough' dev='eth10'/> <interface dev='eth10'/> <interface dev='eth12'/> <interface dev='eth18'/> <interface dev='eth20'/> </forward>
However with an ethernet card with 64 VF's or more, the above method gets tedious on the system.
On the other hand, just parameterizing a string (eth%d) is inadequate, eg, when there are multiple non-contiguous ranges.
Proposed Method: The 4 patches provided:
i) Introduce a new element 'pf' to implicitly create an interface pool of all the Virtual Functions attached to the specified Physical Function. ii) Modify the networkAllocateActualDevice, networkNotifyActualDevice and networkReleaseActualDevice API to use the above mentioned interface pool in the passthrough mode. iii) Allow virsh net-dumpxml to use an option --inactive to differentiate between explicit and implicit interface pools
Hence Libvirt will now support both the methods mentioned below:
* Explicit interface list. App inputs:
<forward mode='passthrough'> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
libvirt does not change XML
* Automatically interface list from PF. App inputs:
<forward mode='passthrough'> <pf dev='eth0'/> </forward>
libvirt expands XML to be
<forward mode='passthrough'> <pf dev='eth0'/> <interface dev='eth10'/> <interface dev='eth11'/> <interface dev='eth12'/> <interface dev='eth13'/> </forward>
In the above case we need to differentiate between the implicit and explicit interface pool, which can be done by comparing the dumpxml from active and inactive domains. This will need the addition of the flag VIR_NETWORK_XML_INACTIVE to virNetworkGetXMLDesc().
This patch series supports the use of option --inactive with virsh net-dumpxml.
The design and code all looks good to me. Just some fairly minor code & style bugs to deal with, which I've replied to inline.
Regards, Daniel
Thanks Daniel. Regards, Shradha Shah
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Shradha Shah