[libvirt PATCH 08/28] util: move/rename virFirewallApplyRuleDirect to virIptablesApplyFirewallRule

Laine Stump posted 28 patches 1 year, 2 months ago
There is a newer version of this series
[libvirt PATCH 08/28] util: move/rename virFirewallApplyRuleDirect to virIptablesApplyFirewallRule
Posted by Laine Stump 1 year, 2 months ago
This is the only iptables-specific function in all of
virfirewall.c. By moving it to viriptables.c (with appropriate
renaming), and calling it indirectly through a similarly named wrapper
function in virnetfilter.c, we have made virfirewall.c backend
agnostic (the new wrapper function will soon be calling either
virIptablesApplyFirewallRule() or (to-be-created)
virNftablesApplyFirewallRule() depending on the backend chosen when
creating the virFirewall object).

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/libvirt_private.syms |  2 ++
 src/util/virfirewall.c   | 72 ++-----------------------------------
 src/util/viriptables.c   | 78 ++++++++++++++++++++++++++++++++++++++++
 src/util/viriptables.h   |  6 ++++
 src/util/virnetfilter.c  | 19 ++++++++++
 src/util/virnetfilter.h  |  3 ++
 6 files changed, 110 insertions(+), 70 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 11b84a866a..cf68e4c942 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2550,6 +2550,7 @@ virInitctlSetRunLevel;
 iptablesAddOutputFixUdpChecksum;
 iptablesRemoveOutputFixUdpChecksum;
 iptablesSetupPrivateChains;
+virIptablesApplyFirewallRule;
 
 
 # util/viriscsi.h
@@ -2949,6 +2950,7 @@ virNetfilterAddTcpInput;
 virNetfilterAddTcpOutput;
 virNetfilterAddUdpInput;
 virNetfilterAddUdpOutput;
+virNetfilterApplyFirewallRule;
 virNetfilterRemoveDontMasquerade;
 virNetfilterRemoveForwardAllowCross;
 virNetfilterRemoveForwardAllowIn;
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index e3ba8f7846..6603fd6341 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -24,6 +24,7 @@
 
 #include "virfirewall.h"
 #include "virfirewalld.h"
+#include "virnetfilter.h"
 #include "viralloc.h"
 #include "virerror.h"
 #include "vircommand.h"
@@ -37,14 +38,6 @@ VIR_LOG_INIT("util.firewall");
 
 typedef struct _virFirewallGroup virFirewallGroup;
 
-VIR_ENUM_DECL(virFirewallLayerCommand);
-VIR_ENUM_IMPL(virFirewallLayerCommand,
-              VIR_FIREWALL_LAYER_LAST,
-              EBTABLES,
-              IPTABLES,
-              IP6TABLES,
-);
-
 struct _virFirewallRule {
     virFirewallLayer layer;
 
@@ -500,67 +493,6 @@ virFirewallRuleToString(const char *cmd,
 }
 
 
-static int
-virFirewallApplyRuleDirect(virFirewallRule *rule,
-                           char **output)
-{
-    size_t i;
-    const char *bin = virFirewallLayerCommandTypeToString(rule->layer);
-    g_autoptr(virCommand) cmd = NULL;
-    g_autofree char *cmdStr = NULL;
-    int status;
-    g_autofree char *error = NULL;
-
-    if (!bin) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Unknown firewall layer %1$d"),
-                       rule->layer);
-        return -1;
-    }
-
-    cmd = virCommandNewArgList(bin, NULL);
-
-    /* lock to assure nobody else is messing with the tables while we are */
-    switch (rule->layer) {
-    case VIR_FIREWALL_LAYER_ETHERNET:
-        virCommandAddArg(cmd, "--concurrent");
-        break;
-    case VIR_FIREWALL_LAYER_IPV4:
-    case VIR_FIREWALL_LAYER_IPV6:
-        virCommandAddArg(cmd, "-w");
-        break;
-    case VIR_FIREWALL_LAYER_LAST:
-        break;
-    }
-
-    for (i = 0; i < rule->argsLen; i++)
-        virCommandAddArg(cmd, rule->args[i]);
-
-    cmdStr = virCommandToString(cmd, false);
-    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
-
-    virCommandSetOutputBuffer(cmd, output);
-    virCommandSetErrorBuffer(cmd, &error);
-
-    if (virCommandRun(cmd, &status) < 0)
-        return -1;
-
-    if (status != 0) {
-        if (virFirewallRuleGetIgnoreErrors(rule)) {
-            VIR_DEBUG("Ignoring error running command");
-        } else {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Failed to apply firewall rules %1$s: %2$s"),
-                           NULLSTR(cmdStr), NULLSTR(error));
-            VIR_FREE(*output);
-            return -1;
-        }
-    }
-
-    return 0;
-}
-
-
 static int
 virFirewallApplyRule(virFirewall *firewall,
                      virFirewallRule *rule)
@@ -568,7 +500,7 @@ virFirewallApplyRule(virFirewall *firewall,
     g_autofree char *output = NULL;
     g_auto(GStrv) lines = NULL;
 
-    if (virFirewallApplyRuleDirect(rule, &output) < 0)
+    if (virNetfilterApplyFirewallRule(firewall, rule, &output) < 0)
         return -1;
 
     if (rule->queryCB && output) {
diff --git a/src/util/viriptables.c b/src/util/viriptables.c
index a0c35887c5..9c7f7790c4 100644
--- a/src/util/viriptables.c
+++ b/src/util/viriptables.c
@@ -31,6 +31,8 @@
 #include "viriptables.h"
 #include "virfirewalld.h"
 #include "virerror.h"
+#include "viralloc.h"
+#include "vircommand.h"
 #include "virlog.h"
 #include "virhash.h"
 #include "virenum.h"
@@ -40,6 +42,19 @@ VIR_LOG_INIT("util.iptables");
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 
+/* iptables backend uses a different program for each layer. This
+ * gives us a convenient function for converting VIR_FIREWALL_LAYER_*
+ * enum from a virFirewallRule into a binary name.
+ */
+VIR_ENUM_DECL(virIptablesLayerCommand);
+VIR_ENUM_IMPL(virIptablesLayerCommand,
+              VIR_FIREWALL_LAYER_LAST,
+              EBTABLES,
+              IPTABLES,
+              IP6TABLES,
+);
+
+
 VIR_ENUM_DECL(virIptablesAction);
 VIR_ENUM_IMPL(virIptablesAction,
               VIR_FIREWALL_ACTION_LAST,
@@ -49,6 +64,69 @@ VIR_ENUM_IMPL(virIptablesAction,
 );
 
 
+int
+virIptablesApplyFirewallRule(virFirewall *firewall G_GNUC_UNUSED,
+                             virFirewallRule *rule,
+                             char **output)
+{
+    virFirewallLayer layer = virFirewallRuleGetLayer(rule);
+    const char *bin = virIptablesLayerCommandTypeToString(layer);
+    g_autoptr(virCommand) cmd = NULL;
+    g_autofree char *cmdStr = NULL;
+    g_autofree char *error = NULL;
+    size_t i, count;
+    int status;
+
+    if (!bin) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unknown firewall layer %1$d"), layer);
+        return -1;
+    }
+
+    cmd = virCommandNewArgList(bin, NULL);
+
+    /* lock to assure nobody else is messing with the tables while we are */
+    switch (layer) {
+    case VIR_FIREWALL_LAYER_ETHERNET:
+        virCommandAddArg(cmd, "--concurrent");
+        break;
+    case VIR_FIREWALL_LAYER_IPV4:
+    case VIR_FIREWALL_LAYER_IPV6:
+        virCommandAddArg(cmd, "-w");
+        break;
+    case VIR_FIREWALL_LAYER_LAST:
+        break;
+    }
+
+    count = virFirewallRuleGetArgCount(rule);
+    for (i = 0; i < count; i++)
+        virCommandAddArg(cmd, virFirewallRuleGetArg(rule, i));
+
+    cmdStr = virCommandToString(cmd, false);
+    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
+
+    virCommandSetOutputBuffer(cmd, output);
+    virCommandSetErrorBuffer(cmd, &error);
+
+    if (virCommandRun(cmd, &status) < 0)
+        return -1;
+
+    if (status != 0) {
+        if (virFirewallRuleGetIgnoreErrors(rule)) {
+            VIR_DEBUG("Ignoring error running command");
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to apply firewall rules %1$s: %2$s"),
+                           NULLSTR(cmdStr), NULLSTR(error));
+            VIR_FREE(*output);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+
 typedef struct {
     const char *parent;
     const char *child;
diff --git a/src/util/viriptables.h b/src/util/viriptables.h
index 17f43a8fa8..990cb2e25d 100644
--- a/src/util/viriptables.h
+++ b/src/util/viriptables.h
@@ -24,6 +24,12 @@
 #include "virfirewall.h"
 #include "virnetfilter.h"
 
+/* virIptablesApplyFirewallRule should be called only from virnetfilter.c */
+int
+virIptablesApplyFirewallRule(virFirewall *firewall,
+                             virFirewallRule *rule,
+                             char **output);
+
 /* These functions are (currently) called directly from the consumer
  * (e.g. the network driver), and only when the iptables backend is
  * selected. (Possibly/probably functions should be added to the
diff --git a/src/util/virnetfilter.c b/src/util/virnetfilter.c
index 10c1a54e26..ba0f292ea9 100644
--- a/src/util/virnetfilter.c
+++ b/src/util/virnetfilter.c
@@ -44,6 +44,25 @@ VIR_LOG_INIT("util.netfilter");
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 
+/**
+ * virNetfilterApplyFirewallRule:
+ * @fw: the virFirewall this rule is part of (currently unused)
+ * @rule: this particular rule
+ * @ignoreErrors: true if errors should be ignored
+ * @output: everything that appears on stdout as a result of applying the rule
+ *
+ * Applies @rule to the host's network filtering. returns 0 on success
+ * -1 on failure.
+ */
+int
+virNetfilterApplyFirewallRule(virFirewall *fw,
+                              virFirewallRule *rule,
+                              char **output)
+{
+    return virIptablesApplyFirewallRule(fw, rule, output);
+}
+
+
 /**
  * virNetfilterAddTcpInput:
  * @ctx: pointer to the IP table context
diff --git a/src/util/virnetfilter.h b/src/util/virnetfilter.h
index b515512ad7..eff047cde0 100644
--- a/src/util/virnetfilter.h
+++ b/src/util/virnetfilter.h
@@ -30,6 +30,9 @@
 #define VIR_NETFILTER_FWD_X_CHAIN "LIBVIRT_FWX"
 #define VIR_NETFILTER_NAT_POSTROUTE_CHAIN "LIBVIRT_PRT"
 
+int              virNetfilterApplyFirewallRule    (virFirewall *fw,
+                                                   virFirewallRule *rule,
+                                                   char **output);
 void             virNetfilterAddTcpInput         (virFirewall *fw,
                                                   virFirewallLayer layer,
                                                   const char *iface,
-- 
2.39.2
Re: [libvirt PATCH 08/28] util: move/rename virFirewallApplyRuleDirect to virIptablesApplyFirewallRule
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Sun, Apr 30, 2023 at 11:19:23PM -0400, Laine Stump wrote:
> This is the only iptables-specific function in all of
> virfirewall.c. By moving it to viriptables.c (with appropriate
> renaming), and calling it indirectly through a similarly named wrapper
> function in virnetfilter.c, we have made virfirewall.c backend
> agnostic (the new wrapper function will soon be calling either
> virIptablesApplyFirewallRule() or (to-be-created)
> virNftablesApplyFirewallRule() depending on the backend chosen when
> creating the virFirewall object).
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virfirewall.c   | 72 ++-----------------------------------
>  src/util/viriptables.c   | 78 ++++++++++++++++++++++++++++++++++++++++
>  src/util/viriptables.h   |  6 ++++
>  src/util/virnetfilter.c  | 19 ++++++++++
>  src/util/virnetfilter.h  |  3 ++

I don't much like this split of responsibilities.

With the current codebase

  * virfirewall.c is the low level transactional interface for
    interacting with firewalls. 

  * viriptables.c is a medium level interface providing helpers
    needed by the network bridge driver

The viriptables.c file probably ought not to even  be located
in the src/util directory. Its API is inherantly tied to the
bridge driver, so ought to be moved to src/network/bridge_iptables.c
I think.

IOW, we have a clean flow from high level to low level of

   bridge_driver.c -> viriptables.c -> virfirewall.c

and

 nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c

After this change, AFAICT we have dependancy loops

  * virfirewall.c is the low level transactional interface for
    interacting with firwalls

  * viriptables.c is a medium level interface providing helpers
    needed by the netfilter APIs, and also helpers needed by
    virfirewall.c

  * virnetfilter.c is a slightly higher level inteface
    providing helpers needed by the bridge interface

IOW, AFAICT we now have

  bridge-driver.c -> virnetfilter.c -> viriptables.c -> virfirewall.c
                                           ^                 |
                                           |                 |
                                           \-----------------/

and

 nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c -> viriptables.c

>  6 files changed, 110 insertions(+), 70 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 11b84a866a..cf68e4c942 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2550,6 +2550,7 @@ virInitctlSetRunLevel;
>  iptablesAddOutputFixUdpChecksum;
>  iptablesRemoveOutputFixUdpChecksum;
>  iptablesSetupPrivateChains;
> +virIptablesApplyFirewallRule;
>  
>  
>  # util/viriscsi.h
> @@ -2949,6 +2950,7 @@ virNetfilterAddTcpInput;
>  virNetfilterAddTcpOutput;
>  virNetfilterAddUdpInput;
>  virNetfilterAddUdpOutput;
> +virNetfilterApplyFirewallRule;
>  virNetfilterRemoveDontMasquerade;
>  virNetfilterRemoveForwardAllowCross;
>  virNetfilterRemoveForwardAllowIn;
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index e3ba8f7846..6603fd6341 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -24,6 +24,7 @@
>  
>  #include "virfirewall.h"
>  #include "virfirewalld.h"
> +#include "virnetfilter.h"
>  #include "viralloc.h"
>  #include "virerror.h"
>  #include "vircommand.h"
> @@ -37,14 +38,6 @@ VIR_LOG_INIT("util.firewall");
>  
>  typedef struct _virFirewallGroup virFirewallGroup;
>  
> -VIR_ENUM_DECL(virFirewallLayerCommand);
> -VIR_ENUM_IMPL(virFirewallLayerCommand,
> -              VIR_FIREWALL_LAYER_LAST,
> -              EBTABLES,
> -              IPTABLES,
> -              IP6TABLES,
> -);
> -
>  struct _virFirewallRule {
>      virFirewallLayer layer;
>  
> @@ -500,67 +493,6 @@ virFirewallRuleToString(const char *cmd,
>  }
>  
>  
> -static int
> -virFirewallApplyRuleDirect(virFirewallRule *rule,
> -                           char **output)
> -{
> -    size_t i;
> -    const char *bin = virFirewallLayerCommandTypeToString(rule->layer);
> -    g_autoptr(virCommand) cmd = NULL;
> -    g_autofree char *cmdStr = NULL;
> -    int status;
> -    g_autofree char *error = NULL;
> -
> -    if (!bin) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Unknown firewall layer %1$d"),
> -                       rule->layer);
> -        return -1;
> -    }
> -
> -    cmd = virCommandNewArgList(bin, NULL);
> -
> -    /* lock to assure nobody else is messing with the tables while we are */
> -    switch (rule->layer) {
> -    case VIR_FIREWALL_LAYER_ETHERNET:
> -        virCommandAddArg(cmd, "--concurrent");
> -        break;
> -    case VIR_FIREWALL_LAYER_IPV4:
> -    case VIR_FIREWALL_LAYER_IPV6:
> -        virCommandAddArg(cmd, "-w");
> -        break;
> -    case VIR_FIREWALL_LAYER_LAST:
> -        break;
> -    }
> -
> -    for (i = 0; i < rule->argsLen; i++)
> -        virCommandAddArg(cmd, rule->args[i]);
> -
> -    cmdStr = virCommandToString(cmd, false);
> -    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> -
> -    virCommandSetOutputBuffer(cmd, output);
> -    virCommandSetErrorBuffer(cmd, &error);
> -
> -    if (virCommandRun(cmd, &status) < 0)
> -        return -1;
> -
> -    if (status != 0) {
> -        if (virFirewallRuleGetIgnoreErrors(rule)) {
> -            VIR_DEBUG("Ignoring error running command");
> -        } else {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Failed to apply firewall rules %1$s: %2$s"),
> -                           NULLSTR(cmdStr), NULLSTR(error));
> -            VIR_FREE(*output);
> -            return -1;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
> -
>  static int
>  virFirewallApplyRule(virFirewall *firewall,
>                       virFirewallRule *rule)
> @@ -568,7 +500,7 @@ virFirewallApplyRule(virFirewall *firewall,
>      g_autofree char *output = NULL;
>      g_auto(GStrv) lines = NULL;
>  
> -    if (virFirewallApplyRuleDirect(rule, &output) < 0)
> +    if (virNetfilterApplyFirewallRule(firewall, rule, &output) < 0)
>          return -1;
>  
>      if (rule->queryCB && output) {
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index a0c35887c5..9c7f7790c4 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -31,6 +31,8 @@
>  #include "viriptables.h"
>  #include "virfirewalld.h"
>  #include "virerror.h"
> +#include "viralloc.h"
> +#include "vircommand.h"
>  #include "virlog.h"
>  #include "virhash.h"
>  #include "virenum.h"
> @@ -40,6 +42,19 @@ VIR_LOG_INIT("util.iptables");
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  
> +/* iptables backend uses a different program for each layer. This
> + * gives us a convenient function for converting VIR_FIREWALL_LAYER_*
> + * enum from a virFirewallRule into a binary name.
> + */
> +VIR_ENUM_DECL(virIptablesLayerCommand);
> +VIR_ENUM_IMPL(virIptablesLayerCommand,
> +              VIR_FIREWALL_LAYER_LAST,
> +              EBTABLES,
> +              IPTABLES,
> +              IP6TABLES,
> +);
> +
> +
>  VIR_ENUM_DECL(virIptablesAction);
>  VIR_ENUM_IMPL(virIptablesAction,
>                VIR_FIREWALL_ACTION_LAST,
> @@ -49,6 +64,69 @@ VIR_ENUM_IMPL(virIptablesAction,
>  );
>  
>  
> +int
> +virIptablesApplyFirewallRule(virFirewall *firewall G_GNUC_UNUSED,
> +                             virFirewallRule *rule,
> +                             char **output)
> +{
> +    virFirewallLayer layer = virFirewallRuleGetLayer(rule);
> +    const char *bin = virIptablesLayerCommandTypeToString(layer);
> +    g_autoptr(virCommand) cmd = NULL;
> +    g_autofree char *cmdStr = NULL;
> +    g_autofree char *error = NULL;
> +    size_t i, count;
> +    int status;
> +
> +    if (!bin) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unknown firewall layer %1$d"), layer);
> +        return -1;
> +    }
> +
> +    cmd = virCommandNewArgList(bin, NULL);
> +
> +    /* lock to assure nobody else is messing with the tables while we are */
> +    switch (layer) {
> +    case VIR_FIREWALL_LAYER_ETHERNET:
> +        virCommandAddArg(cmd, "--concurrent");
> +        break;
> +    case VIR_FIREWALL_LAYER_IPV4:
> +    case VIR_FIREWALL_LAYER_IPV6:
> +        virCommandAddArg(cmd, "-w");
> +        break;
> +    case VIR_FIREWALL_LAYER_LAST:
> +        break;
> +    }
> +
> +    count = virFirewallRuleGetArgCount(rule);
> +    for (i = 0; i < count; i++)
> +        virCommandAddArg(cmd, virFirewallRuleGetArg(rule, i));
> +
> +    cmdStr = virCommandToString(cmd, false);
> +    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> +
> +    virCommandSetOutputBuffer(cmd, output);
> +    virCommandSetErrorBuffer(cmd, &error);
> +
> +    if (virCommandRun(cmd, &status) < 0)
> +        return -1;
> +
> +    if (status != 0) {
> +        if (virFirewallRuleGetIgnoreErrors(rule)) {
> +            VIR_DEBUG("Ignoring error running command");
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to apply firewall rules %1$s: %2$s"),
> +                           NULLSTR(cmdStr), NULLSTR(error));
> +            VIR_FREE(*output);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  typedef struct {
>      const char *parent;
>      const char *child;
> diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> index 17f43a8fa8..990cb2e25d 100644
> --- a/src/util/viriptables.h
> +++ b/src/util/viriptables.h
> @@ -24,6 +24,12 @@
>  #include "virfirewall.h"
>  #include "virnetfilter.h"
>  
> +/* virIptablesApplyFirewallRule should be called only from virnetfilter.c */
> +int
> +virIptablesApplyFirewallRule(virFirewall *firewall,
> +                             virFirewallRule *rule,
> +                             char **output);
> +
>  /* These functions are (currently) called directly from the consumer
>   * (e.g. the network driver), and only when the iptables backend is
>   * selected. (Possibly/probably functions should be added to the
> diff --git a/src/util/virnetfilter.c b/src/util/virnetfilter.c
> index 10c1a54e26..ba0f292ea9 100644
> --- a/src/util/virnetfilter.c
> +++ b/src/util/virnetfilter.c
> @@ -44,6 +44,25 @@ VIR_LOG_INIT("util.netfilter");
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  
> +/**
> + * virNetfilterApplyFirewallRule:
> + * @fw: the virFirewall this rule is part of (currently unused)
> + * @rule: this particular rule
> + * @ignoreErrors: true if errors should be ignored
> + * @output: everything that appears on stdout as a result of applying the rule
> + *
> + * Applies @rule to the host's network filtering. returns 0 on success
> + * -1 on failure.
> + */
> +int
> +virNetfilterApplyFirewallRule(virFirewall *fw,
> +                              virFirewallRule *rule,
> +                              char **output)
> +{
> +    return virIptablesApplyFirewallRule(fw, rule, output);
> +}
> +
> +
>  /**
>   * virNetfilterAddTcpInput:
>   * @ctx: pointer to the IP table context
> diff --git a/src/util/virnetfilter.h b/src/util/virnetfilter.h
> index b515512ad7..eff047cde0 100644
> --- a/src/util/virnetfilter.h
> +++ b/src/util/virnetfilter.h
> @@ -30,6 +30,9 @@
>  #define VIR_NETFILTER_FWD_X_CHAIN "LIBVIRT_FWX"
>  #define VIR_NETFILTER_NAT_POSTROUTE_CHAIN "LIBVIRT_PRT"
>  
> +int              virNetfilterApplyFirewallRule    (virFirewall *fw,
> +                                                   virFirewallRule *rule,
> +                                                   char **output);
>  void             virNetfilterAddTcpInput         (virFirewall *fw,
>                                                    virFirewallLayer layer,
>                                                    const char *iface,
> -- 
> 2.39.2
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 08/28] util: move/rename virFirewallApplyRuleDirect to virIptablesApplyFirewallRule
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Wed, May 03, 2023 at 04:21:28PM +0100, Daniel P. Berrangé wrote:
> On Sun, Apr 30, 2023 at 11:19:23PM -0400, Laine Stump wrote:
> > This is the only iptables-specific function in all of
> > virfirewall.c. By moving it to viriptables.c (with appropriate
> > renaming), and calling it indirectly through a similarly named wrapper
> > function in virnetfilter.c, we have made virfirewall.c backend
> > agnostic (the new wrapper function will soon be calling either
> > virIptablesApplyFirewallRule() or (to-be-created)
> > virNftablesApplyFirewallRule() depending on the backend chosen when
> > creating the virFirewall object).
> > 
> > Signed-off-by: Laine Stump <laine@redhat.com>
> > ---
> >  src/libvirt_private.syms |  2 ++
> >  src/util/virfirewall.c   | 72 ++-----------------------------------
> >  src/util/viriptables.c   | 78 ++++++++++++++++++++++++++++++++++++++++
> >  src/util/viriptables.h   |  6 ++++
> >  src/util/virnetfilter.c  | 19 ++++++++++
> >  src/util/virnetfilter.h  |  3 ++
> 
> I don't much like this split of responsibilities.
> 
> With the current codebase
> 
>   * virfirewall.c is the low level transactional interface for
>     interacting with firewalls. 
> 
>   * viriptables.c is a medium level interface providing helpers
>     needed by the network bridge driver
> 
> The viriptables.c file probably ought not to even  be located
> in the src/util directory. Its API is inherantly tied to the
> bridge driver, so ought to be moved to src/network/bridge_iptables.c
> I think.
> 
> IOW, we have a clean flow from high level to low level of
> 
>    bridge_driver.c -> viriptables.c -> virfirewall.c
> 
> and
> 
>  nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c
> 
> After this change, AFAICT we have dependancy loops
> 
>   * virfirewall.c is the low level transactional interface for
>     interacting with firwalls
> 
>   * viriptables.c is a medium level interface providing helpers
>     needed by the netfilter APIs, and also helpers needed by
>     virfirewall.c
> 
>   * virnetfilter.c is a slightly higher level inteface
>     providing helpers needed by the bridge interface
> 
> IOW, AFAICT we now have
> 
>   bridge-driver.c -> virnetfilter.c -> viriptables.c -> virfirewall.c
>                                            ^                 |
>                                            |                 |
>                                            \-----------------/
> 
> and
> 
>  nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c -> viriptables.c

Looking at the overall  end of this series, IMHO, we can just
drop this patch without any problem. The function that is being
moved here does not rely on any of the other code that exists
in the iptables.c file, and does rely on stuff in virfirewall.c

The only reason to move it appears to be because the logic is
related to iptables, and I don't think that's compellling when
the downside is creatin of a circular dependancy.

> 
> >  6 files changed, 110 insertions(+), 70 deletions(-)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 11b84a866a..cf68e4c942 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2550,6 +2550,7 @@ virInitctlSetRunLevel;
> >  iptablesAddOutputFixUdpChecksum;
> >  iptablesRemoveOutputFixUdpChecksum;
> >  iptablesSetupPrivateChains;
> > +virIptablesApplyFirewallRule;
> >  
> >  
> >  # util/viriscsi.h
> > @@ -2949,6 +2950,7 @@ virNetfilterAddTcpInput;
> >  virNetfilterAddTcpOutput;
> >  virNetfilterAddUdpInput;
> >  virNetfilterAddUdpOutput;
> > +virNetfilterApplyFirewallRule;
> >  virNetfilterRemoveDontMasquerade;
> >  virNetfilterRemoveForwardAllowCross;
> >  virNetfilterRemoveForwardAllowIn;
> > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > index e3ba8f7846..6603fd6341 100644
> > --- a/src/util/virfirewall.c
> > +++ b/src/util/virfirewall.c
> > @@ -24,6 +24,7 @@
> >  
> >  #include "virfirewall.h"
> >  #include "virfirewalld.h"
> > +#include "virnetfilter.h"
> >  #include "viralloc.h"
> >  #include "virerror.h"
> >  #include "vircommand.h"
> > @@ -37,14 +38,6 @@ VIR_LOG_INIT("util.firewall");
> >  
> >  typedef struct _virFirewallGroup virFirewallGroup;
> >  
> > -VIR_ENUM_DECL(virFirewallLayerCommand);
> > -VIR_ENUM_IMPL(virFirewallLayerCommand,
> > -              VIR_FIREWALL_LAYER_LAST,
> > -              EBTABLES,
> > -              IPTABLES,
> > -              IP6TABLES,
> > -);
> > -
> >  struct _virFirewallRule {
> >      virFirewallLayer layer;
> >  
> > @@ -500,67 +493,6 @@ virFirewallRuleToString(const char *cmd,
> >  }
> >  
> >  
> > -static int
> > -virFirewallApplyRuleDirect(virFirewallRule *rule,
> > -                           char **output)
> > -{
> > -    size_t i;
> > -    const char *bin = virFirewallLayerCommandTypeToString(rule->layer);
> > -    g_autoptr(virCommand) cmd = NULL;
> > -    g_autofree char *cmdStr = NULL;
> > -    int status;
> > -    g_autofree char *error = NULL;
> > -
> > -    if (!bin) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("Unknown firewall layer %1$d"),
> > -                       rule->layer);
> > -        return -1;
> > -    }
> > -
> > -    cmd = virCommandNewArgList(bin, NULL);
> > -
> > -    /* lock to assure nobody else is messing with the tables while we are */
> > -    switch (rule->layer) {
> > -    case VIR_FIREWALL_LAYER_ETHERNET:
> > -        virCommandAddArg(cmd, "--concurrent");
> > -        break;
> > -    case VIR_FIREWALL_LAYER_IPV4:
> > -    case VIR_FIREWALL_LAYER_IPV6:
> > -        virCommandAddArg(cmd, "-w");
> > -        break;
> > -    case VIR_FIREWALL_LAYER_LAST:
> > -        break;
> > -    }
> > -
> > -    for (i = 0; i < rule->argsLen; i++)
> > -        virCommandAddArg(cmd, rule->args[i]);
> > -
> > -    cmdStr = virCommandToString(cmd, false);
> > -    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> > -
> > -    virCommandSetOutputBuffer(cmd, output);
> > -    virCommandSetErrorBuffer(cmd, &error);
> > -
> > -    if (virCommandRun(cmd, &status) < 0)
> > -        return -1;
> > -
> > -    if (status != 0) {
> > -        if (virFirewallRuleGetIgnoreErrors(rule)) {
> > -            VIR_DEBUG("Ignoring error running command");
> > -        } else {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                           _("Failed to apply firewall rules %1$s: %2$s"),
> > -                           NULLSTR(cmdStr), NULLSTR(error));
> > -            VIR_FREE(*output);
> > -            return -1;
> > -        }
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> > -
> >  static int
> >  virFirewallApplyRule(virFirewall *firewall,
> >                       virFirewallRule *rule)
> > @@ -568,7 +500,7 @@ virFirewallApplyRule(virFirewall *firewall,
> >      g_autofree char *output = NULL;
> >      g_auto(GStrv) lines = NULL;
> >  
> > -    if (virFirewallApplyRuleDirect(rule, &output) < 0)
> > +    if (virNetfilterApplyFirewallRule(firewall, rule, &output) < 0)
> >          return -1;
> >  
> >      if (rule->queryCB && output) {
> > diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> > index a0c35887c5..9c7f7790c4 100644
> > --- a/src/util/viriptables.c
> > +++ b/src/util/viriptables.c
> > @@ -31,6 +31,8 @@
> >  #include "viriptables.h"
> >  #include "virfirewalld.h"
> >  #include "virerror.h"
> > +#include "viralloc.h"
> > +#include "vircommand.h"
> >  #include "virlog.h"
> >  #include "virhash.h"
> >  #include "virenum.h"
> > @@ -40,6 +42,19 @@ VIR_LOG_INIT("util.iptables");
> >  #define VIR_FROM_THIS VIR_FROM_NONE
> >  
> >  
> > +/* iptables backend uses a different program for each layer. This
> > + * gives us a convenient function for converting VIR_FIREWALL_LAYER_*
> > + * enum from a virFirewallRule into a binary name.
> > + */
> > +VIR_ENUM_DECL(virIptablesLayerCommand);
> > +VIR_ENUM_IMPL(virIptablesLayerCommand,
> > +              VIR_FIREWALL_LAYER_LAST,
> > +              EBTABLES,
> > +              IPTABLES,
> > +              IP6TABLES,
> > +);
> > +
> > +
> >  VIR_ENUM_DECL(virIptablesAction);
> >  VIR_ENUM_IMPL(virIptablesAction,
> >                VIR_FIREWALL_ACTION_LAST,
> > @@ -49,6 +64,69 @@ VIR_ENUM_IMPL(virIptablesAction,
> >  );
> >  
> >  
> > +int
> > +virIptablesApplyFirewallRule(virFirewall *firewall G_GNUC_UNUSED,
> > +                             virFirewallRule *rule,
> > +                             char **output)
> > +{
> > +    virFirewallLayer layer = virFirewallRuleGetLayer(rule);
> > +    const char *bin = virIptablesLayerCommandTypeToString(layer);
> > +    g_autoptr(virCommand) cmd = NULL;
> > +    g_autofree char *cmdStr = NULL;
> > +    g_autofree char *error = NULL;
> > +    size_t i, count;
> > +    int status;
> > +
> > +    if (!bin) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("Unknown firewall layer %1$d"), layer);
> > +        return -1;
> > +    }
> > +
> > +    cmd = virCommandNewArgList(bin, NULL);
> > +
> > +    /* lock to assure nobody else is messing with the tables while we are */
> > +    switch (layer) {
> > +    case VIR_FIREWALL_LAYER_ETHERNET:
> > +        virCommandAddArg(cmd, "--concurrent");
> > +        break;
> > +    case VIR_FIREWALL_LAYER_IPV4:
> > +    case VIR_FIREWALL_LAYER_IPV6:
> > +        virCommandAddArg(cmd, "-w");
> > +        break;
> > +    case VIR_FIREWALL_LAYER_LAST:
> > +        break;
> > +    }
> > +
> > +    count = virFirewallRuleGetArgCount(rule);
> > +    for (i = 0; i < count; i++)
> > +        virCommandAddArg(cmd, virFirewallRuleGetArg(rule, i));
> > +
> > +    cmdStr = virCommandToString(cmd, false);
> > +    VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> > +
> > +    virCommandSetOutputBuffer(cmd, output);
> > +    virCommandSetErrorBuffer(cmd, &error);
> > +
> > +    if (virCommandRun(cmd, &status) < 0)
> > +        return -1;
> > +
> > +    if (status != 0) {
> > +        if (virFirewallRuleGetIgnoreErrors(rule)) {
> > +            VIR_DEBUG("Ignoring error running command");
> > +        } else {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("Failed to apply firewall rules %1$s: %2$s"),
> > +                           NULLSTR(cmdStr), NULLSTR(error));
> > +            VIR_FREE(*output);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> >  typedef struct {
> >      const char *parent;
> >      const char *child;
> > diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> > index 17f43a8fa8..990cb2e25d 100644
> > --- a/src/util/viriptables.h
> > +++ b/src/util/viriptables.h
> > @@ -24,6 +24,12 @@
> >  #include "virfirewall.h"
> >  #include "virnetfilter.h"
> >  
> > +/* virIptablesApplyFirewallRule should be called only from virnetfilter.c */
> > +int
> > +virIptablesApplyFirewallRule(virFirewall *firewall,
> > +                             virFirewallRule *rule,
> > +                             char **output);
> > +
> >  /* These functions are (currently) called directly from the consumer
> >   * (e.g. the network driver), and only when the iptables backend is
> >   * selected. (Possibly/probably functions should be added to the
> > diff --git a/src/util/virnetfilter.c b/src/util/virnetfilter.c
> > index 10c1a54e26..ba0f292ea9 100644
> > --- a/src/util/virnetfilter.c
> > +++ b/src/util/virnetfilter.c
> > @@ -44,6 +44,25 @@ VIR_LOG_INIT("util.netfilter");
> >  #define VIR_FROM_THIS VIR_FROM_NONE
> >  
> >  
> > +/**
> > + * virNetfilterApplyFirewallRule:
> > + * @fw: the virFirewall this rule is part of (currently unused)
> > + * @rule: this particular rule
> > + * @ignoreErrors: true if errors should be ignored
> > + * @output: everything that appears on stdout as a result of applying the rule
> > + *
> > + * Applies @rule to the host's network filtering. returns 0 on success
> > + * -1 on failure.
> > + */
> > +int
> > +virNetfilterApplyFirewallRule(virFirewall *fw,
> > +                              virFirewallRule *rule,
> > +                              char **output)
> > +{
> > +    return virIptablesApplyFirewallRule(fw, rule, output);
> > +}
> > +
> > +
> >  /**
> >   * virNetfilterAddTcpInput:
> >   * @ctx: pointer to the IP table context
> > diff --git a/src/util/virnetfilter.h b/src/util/virnetfilter.h
> > index b515512ad7..eff047cde0 100644
> > --- a/src/util/virnetfilter.h
> > +++ b/src/util/virnetfilter.h
> > @@ -30,6 +30,9 @@
> >  #define VIR_NETFILTER_FWD_X_CHAIN "LIBVIRT_FWX"
> >  #define VIR_NETFILTER_NAT_POSTROUTE_CHAIN "LIBVIRT_PRT"
> >  
> > +int              virNetfilterApplyFirewallRule    (virFirewall *fw,
> > +                                                   virFirewallRule *rule,
> > +                                                   char **output);
> >  void             virNetfilterAddTcpInput         (virFirewall *fw,
> >                                                    virFirewallLayer layer,
> >                                                    const char *iface,
> > -- 
> > 2.39.2
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH 08/28] util: move/rename virFirewallApplyRuleDirect to virIptablesApplyFirewallRule
Posted by Laine Stump 1 year, 2 months ago
On 5/3/23 12:05 PM, Daniel P. Berrangé wrote:
> On Wed, May 03, 2023 at 04:21:28PM +0100, Daniel P. Berrangé wrote:
>> On Sun, Apr 30, 2023 at 11:19:23PM -0400, Laine Stump wrote:
>>> This is the only iptables-specific function in all of
>>> virfirewall.c. By moving it to viriptables.c (with appropriate
>>> renaming), and calling it indirectly through a similarly named wrapper
>>> function in virnetfilter.c, we have made virfirewall.c backend
>>> agnostic (the new wrapper function will soon be calling either
>>> virIptablesApplyFirewallRule() or (to-be-created)
>>> virNftablesApplyFirewallRule() depending on the backend chosen when
>>> creating the virFirewall object).
>>>
>>> Signed-off-by: Laine Stump <laine@redhat.com>
>>> ---
>>>   src/libvirt_private.syms |  2 ++
>>>   src/util/virfirewall.c   | 72 ++-----------------------------------
>>>   src/util/viriptables.c   | 78 ++++++++++++++++++++++++++++++++++++++++
>>>   src/util/viriptables.h   |  6 ++++
>>>   src/util/virnetfilter.c  | 19 ++++++++++
>>>   src/util/virnetfilter.h  |  3 ++
>>
>> I don't much like this split of responsibilities.
>>
>> With the current codebase
>>
>>    * virfirewall.c is the low level transactional interface for
>>      interacting with firewalls.
>>
>>    * viriptables.c is a medium level interface providing helpers
>>      needed by the network bridge driver
>>
>> The viriptables.c file probably ought not to even  be located
>> in the src/util directory. Its API is inherantly tied to the
>> bridge driver, so ought to be moved to src/network/bridge_iptables.c
>> I think.
>>
>> IOW, we have a clean flow from high level to low level of
>>
>>     bridge_driver.c -> viriptables.c -> virfirewall.c
>>
>> and
>>
>>   nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c
>>
>> After this change, AFAICT we have dependancy loops
>>
>>    * virfirewall.c is the low level transactional interface for
>>      interacting with firwalls
>>
>>    * viriptables.c is a medium level interface providing helpers
>>      needed by the netfilter APIs, and also helpers needed by
>>      virfirewall.c
>>
>>    * virnetfilter.c is a slightly higher level inteface
>>      providing helpers needed by the bridge interface
>>
>> IOW, AFAICT we now have
>>
>>    bridge-driver.c -> virnetfilter.c -> viriptables.c -> virfirewall.c
>>                                             ^                 |
>>                                             |                 |
>>                                             \-----------------/
>>
>> and
>>
>>   nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c -> viriptables.c

Well done! You've caught the bit of the code that I felt the most 
uncomfortable about and mapped it out in a way that I can no longer 
gloss it over :-).


> 
> Looking at the overall  end of this series, IMHO, we can just
> drop this patch without any problem. The function that is being
> moved here does not rely on any of the other code that exists
> in the iptables.c file, and does rely on stuff in virfirewall.c
> 
> The only reason to move it appears to be because the logic is
> related to iptables, and I don't think that's compellling when
> the downside is creatin of a circular dependancy.

Well, there is more difference between virIptabledApplyFirewallRule() 
and virNftablesApplyFirewallRule() by the time you get to the end of the 
series - patches 20/28 and 21/28 add in the code that automatically 
generates a rollback rule (the command needed to remove the rule that is 
being added).

I haven't fully considered your comments on 18/28 yet, but it sounds 
like you think we don't need to automatically generate these rules, 
which would make for less difference between the backends. I still don't 
like the idea of *not* auto-generating rollback/removal rules when 
they're needed. Maybe the circular dependency could be eliminated by 
passing virFirewallApplRule a function that should be called to generate 
the rollback rule; this pointer would be NULL in the cases that a 
removal rule was unnecessary. (I'll try to avoid anything like that - 
simpler is better)