[libvirt] [PATCHv2 5/9] conf: support abstracted interface info in network XML

[Eric Blake]

On 07/20/2011 12:21 AM, Laine Stump wrote:
> The network XML is updated in the following ways:
>
> 1) The<forward>  element can now contain a list of forward interfaces:
>
>       <forward ....>
>         <interface dev='eth10'/>
>         <interface dev='eth11'/>
>         <interface dev='eth12'/>
>         <interface dev='eth13'/>
>       </forward>
>
>     The first of these takes the place of the dev attribute that is
>     normally in<forward>  - on when defining a network you can specify
>     either one, and on output both will be present. If you specify
>     both, they must match.

>
> Note that I now have text cases for both this and the domain XML
> changes, and documentation for the domain XML changes. I am still
> missing documentation for the network XML, and am working on that
> while you're reviewing this code.

And you know I'll bug you for it if it hasn't shown up by next week :)

> +        case VIR_NETWORK_FORWARD_PASSTHROUGH:
> +            if (def->bridge) {
> +                virNetworkReportError(VIR_ERR_XML_ERROR,
> +                                      _("bridge name not allowed in %s mode (network '%s'"),
> +                                      virNetworkForwardTypeToString(def->forwardType),
> +                                      def->name);
> +                goto error;
> +            }
> +            /* fall through to next case */
> +        case VIR_NETWORK_FORWARD_BRIDGE:

We'll soon find out whether Coverity understands this comment. 
Hopefully yes.

> +++ b/src/libvirt_private.syms
> @@ -748,6 +748,7 @@ virNetworkRemoveInactive;
>   virNetworkSaveConfig;
>   virNetworkSetBridgeMacAddr;
>   virNetworkSetBridgeName;
> +virPortGroupFindByName;
>
>   # node_device_conf.h

Simple merge conflict here if you applied my squash changes from patch 4/9.

> @@ -959,11 +960,12 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
>       if (iptablesAddForwardMasquerade(driver->iptables,
>                                        &ipdef->address,
>                                        prefix,
> -                                     network->def->forwardDev,
> +                                     forwardIf,
>                                        NULL)<  0) {
>           networkReportError(VIR_ERR_SYSTEM_ERROR,
> -                           _("failed to add iptables rule to enable masquerading to '%s'"),
> -                           network->def->forwardDev ? network->def->forwardDev : NULL);
> +                           _("failed to add iptables rule to enable masquerading%s%s"),
> +                           forwardIf ? " to " : "",
> +                           forwardIf ? forwardIf : "");

Translators don't like this.  Not to mention that " to " is not marked 
for translation, so you'd get a mixed-language result.

Better to spell out two possible sentences.

>           goto masqerr3;
>       }
>
> @@ -971,11 +973,12 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
>       if (iptablesAddForwardMasquerade(driver->iptables,
>                                        &ipdef->address,
>                                        prefix,
> -                                     network->def->forwardDev,
> +                                     forwardIf,
>                                        "udp")<  0) {
>           networkReportError(VIR_ERR_SYSTEM_ERROR,
> -                           _("failed to add iptables rule to enable UDP masquerading to '%s'"),
> -                           network->def->forwardDev ? network->def->forwardDev : NULL);
> +                           _("failed to add iptables rule to enable UDP masquerading%s%s"),
> +                           forwardIf ? " to " : "",
> +                           forwardIf ? forwardIf : "");

Likewise.

ACK with this squashed in (or something similar, if you don't like my 
abuse of the fact that printf ignores an unused argument on one of the 
two strings):

diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
index 4459146..402f447 100644
--- i/src/network/bridge_driver.c
+++ w/src/network/bridge_driver.c
@@ -963,9 +963,10 @@ networkAddMasqueradingIptablesRules(struct 
network_driver *driver,
                                       forwardIf,
                                       NULL) < 0) {
          networkReportError(VIR_ERR_SYSTEM_ERROR,
-                           _("failed to add iptables rule to enable 
masquerading%s%s"),
-                           forwardIf ? " to " : "",
-                           forwardIf ? forwardIf : "");
+                           forwardIf ?
+                           _("failed to add iptables rule to enable 
masquerading to %s") :
+                           _("failed to add iptables rule to enable 
masquerading"),
+                           forwardIf);
          goto masqerr3;
      }

@@ -976,9 +977,10 @@ networkAddMasqueradingIptablesRules(struct 
network_driver *driver,
                                       forwardIf,
                                       "udp") < 0) {
          networkReportError(VIR_ERR_SYSTEM_ERROR,
-                           _("failed to add iptables rule to enable UDP 
masquerading%s%s"),
-                           forwardIf ? " to " : "",
-                           forwardIf ? forwardIf : "");
+                           forwardIf ?
+                           _("failed to add iptables rule to enable UDP 
masquerading to %s") :
+                           _("failed to add iptables rule to enable UDP 
masquerading"),
+                           forwardIf);
          goto masqerr4;
      }

@@ -989,9 +991,10 @@ networkAddMasqueradingIptablesRules(struct 
network_driver *driver,
                                       forwardIf,
                                       "tcp") < 0) {
          networkReportError(VIR_ERR_SYSTEM_ERROR,
-                           _("failed to add iptables rule to enable TCP 
masquerading%s%s"),
-                           forwardIf ? " to " : "",
-                           forwardIf ? forwardIf : "");
+                           forwardIf ?
+                           _("failed to add iptables rule to enable TCP 
masquerading to %s") :
+                           _("failed to add iptables rule to enable TCP 
masquerading"),
+                           forwardIf);
          goto masqerr5;
      }



-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org