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