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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux