[libvirt] [PATCH 3/5 v3] Adding the element pf to network xml.

[Daniel P. Berrange]

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