[libvirt] [PATCH] util: refactor iptables command construction into multiple steps

[Natanael Copa]

Instead of creating an iptables command in one shot, do it in steps
so we can add conditional options like physdev and protocol.

This removes code duplication while keeping existing behaviour.

Signed-off-by: Natanael Copa <ncopa at alpinelinux.org>
---

This started with me wanting to add support for setting the public ip source
address when network mode='nat' and there are multiple public ip addresses
on the external interface.

On IRC we talked about adding an option in the xml like this:
<network>
  <forward mode='nat' publicaddr='n.n.n.n'/>
</network>

Which would make iptables use '-j SNAT --to-source n.n.n.n' instead of
'-j MASQUERADE'.

However with the current approach we would need to construct 8 different
iptables command combinations, hence the need for refactoring.

Thanks!


 src/util/iptables.c | 130 +++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 72 deletions(-)

diff --git a/src/util/iptables.c b/src/util/iptables.c
index 00a1c29..407ca3a 100644
--- a/src/util/iptables.c
+++ b/src/util/iptables.c
@@ -127,15 +127,10 @@ iptRulesNew(const char *table,
     return NULL;
 }
 
-static int ATTRIBUTE_SENTINEL
-iptablesAddRemoveRule(iptRules *rules, int family, int action,
-                      const char *arg, ...)
+static virCommandPtr
+iptablesCommandNew(iptRules *rules, int family, int action)
 {
-    va_list args;
-    int ret;
     virCommandPtr cmd = NULL;
-    const char *s;
-
 #if HAVE_FIREWALLD
     virIpTablesInitialize();
     if (firewall_cmd_path) {
@@ -152,16 +147,36 @@ iptablesAddRemoveRule(iptRules *rules, int family, int action,
 
     virCommandAddArgList(cmd, "--table", rules->table,
                          action == ADD ? "--insert" : "--delete",
-                         rules->chain, arg, NULL);
+                         rules->chain, NULL);
+    return cmd;
+}
+
+static int
+iptablesCommandRunAndFree(virCommandPtr cmd)
+{
+    int ret;
+    ret = virCommandRun(cmd, NULL);
+    virCommandFree(cmd);
+    return ret;
+}
+
+static int ATTRIBUTE_SENTINEL
+iptablesAddRemoveRule(iptRules *rules, int family, int action,
+                      const char *arg, ...)
+{
+    va_list args;
+    virCommandPtr cmd = NULL;
+    const char *s;
+
+    cmd = iptablesCommandNew(rules, family, action);
+    virCommandAddArg(cmd, arg);
 
     va_start(args, arg);
     while ((s = va_arg(args, const char *)))
         virCommandAddArg(cmd, s);
     va_end(args);
 
-    ret = virCommandRun(cmd, NULL);
-    virCommandFree(cmd);
-    return ret;
+    return iptablesCommandRunAndFree(cmd);
 }
 
 /**
@@ -370,28 +385,24 @@ iptablesForwardAllowOut(iptablesContext *ctx,
 {
     int ret;
     char *networkstr;
+    virCommandPtr cmd = NULL;
 
     if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
         return -1;
 
-    if (physdev && physdev[0]) {
-        ret = iptablesAddRemoveRule(ctx->forward_filter,
-                                    VIR_SOCKET_ADDR_FAMILY(netaddr),
-                                    action,
-                                    "--source", networkstr,
-                                    "--in-interface", iface,
-                                    "--out-interface", physdev,
-                                    "--jump", "ACCEPT",
-                                    NULL);
-    } else {
-        ret = iptablesAddRemoveRule(ctx->forward_filter,
-                                    VIR_SOCKET_ADDR_FAMILY(netaddr),
-                                    action,
-                                    "--source", networkstr,
-                                    "--in-interface", iface,
-                                    "--jump", "ACCEPT",
-                                    NULL);
-    }
+    cmd = iptablesCommandNew(ctx->forward_filter,
+                             VIR_SOCKET_ADDR_FAMILY(netaddr),
+                             action);
+    virCommandAddArgList(cmd,
+                         "--source", networkstr,
+			 "--in-interface", iface, NULL);
+
+    if (physdev && physdev[0])
+        virCommandAddArgList(cmd, "--out-interface", physdev, NULL);
+
+    virCommandAddArgList(cmd, "--jump", "ACCEPT", NULL);
+
+    ret = iptablesCommandRunAndFree(cmd);
     VIR_FREE(networkstr);
     return ret;
 }
@@ -797,6 +808,7 @@ iptablesForwardMasquerade(iptablesContext *ctx,
 {
     int ret;
     char *networkstr;
+    virCommandPtr cmd = NULL;
 
     if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
         return -1;
@@ -810,49 +822,23 @@ iptablesForwardMasquerade(iptablesContext *ctx,
         return -1;
     }
 
-    if (protocol && protocol[0]) {
-        if (physdev && physdev[0]) {
-            ret = iptablesAddRemoveRule(ctx->nat_postrouting,
-                                        AF_INET,
-                                        action,
-                                        "--source", networkstr,
-                                        "-p", protocol,
-                                        "!", "--destination", networkstr,
-                                        "--out-interface", physdev,
-                                        "--jump", "MASQUERADE",
-                                        "--to-ports", "1024-65535",
-                                        NULL);
-        } else {
-            ret = iptablesAddRemoveRule(ctx->nat_postrouting,
-                                        AF_INET,
-                                        action,
-                                        "--source", networkstr,
-                                        "-p", protocol,
-                                        "!", "--destination", networkstr,
-                                        "--jump", "MASQUERADE",
-                                        "--to-ports", "1024-65535",
-                                        NULL);
-        }
-    } else {
-        if (physdev && physdev[0]) {
-            ret = iptablesAddRemoveRule(ctx->nat_postrouting,
-                                        AF_INET,
-                                        action,
-                                        "--source", networkstr,
-                                        "!", "--destination", networkstr,
-                                        "--out-interface", physdev,
-                                        "--jump", "MASQUERADE",
-                                        NULL);
-        } else {
-            ret = iptablesAddRemoveRule(ctx->nat_postrouting,
-                                        AF_INET,
-                                        action,
-                                        "--source", networkstr,
-                                        "!", "--destination", networkstr,
-                                        "--jump", "MASQUERADE",
-                                        NULL);
-        }
-    }
+    cmd = iptablesCommandNew(ctx->nat_postrouting, AF_INET, action);
+    virCommandAddArgList(cmd, "--source", networkstr, NULL);
+
+    if (protocol && protocol[0])
+        virCommandAddArgList(cmd, "-p", protocol, NULL);
+
+    virCommandAddArgList(cmd, "!", "--destination", networkstr, NULL);
+
+    if (physdev && physdev[0])
+        virCommandAddArgList(cmd, "--out-interface", physdev, NULL);
+
+    virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL);
+
+    if (protocol && protocol[0])
+        virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL);
+
+    ret = iptablesCommandRunAndFree(cmd);
     VIR_FREE(networkstr);
     return ret;
 }
-- 
1.8.0