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 :|