Re: [PATCH 4/5] util: add new "raw" layer for virFirewallCmd objects

On Fri, Nov 22, 2024 at 04:16:38PM -0500, Laine Stump wrote: > If the layer of a FirewallCmd is "raw", then the first arg is the name > of an arbitrary binary to exec, and the rest are the arguments to that > binary. > > raw layer doesn't support auto-rollback command creation (any rollback > needs to be added manually with virFirewallAddRollbackCmd()), and also > raw layer isn't supported by the iptables backend (it would have been > straightforward to add, but the iptables backend doesn't need it, and > I didn't want to take the chance of causing a regression in that > code for no good reason). I guess the obvious question to ask is why you chose to define a "raw" layer, as opposed to defining a "tc" layer ? Being more targetted about the anticipated usage feels better IMHO.

> > Signed-off-by: Laine Stump <laine(a)redhat.com&gt; > --- > src/network/network_nftables.c | 1 + > src/util/virfirewall.c | 74 +++++++++++++++++++++------------- > src/util/virfirewall.h | 1 + > src/util/virfirewalld.c | 1 + > 4 files changed, 49 insertions(+), 28 deletions(-) > > diff --git a/src/network/network_nftables.c b/src/network/network_nftables.c > index f8b5ab665d..e7ee3cd856 100644 > --- a/src/network/network_nftables.c > +++ b/src/network/network_nftables.c > @@ -71,6 +71,7 @@ VIR_ENUM_DECL(nftablesLayer); > VIR_ENUM_IMPL(nftablesLayer, > VIR_FIREWALL_LAYER_LAST, > "", > + "", > "ip", > "ip6", > ); > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c > index 811b787ecc..48b83715d0 100644 > --- a/src/util/virfirewall.c > +++ b/src/util/virfirewall.c > @@ -44,6 +44,7 @@ VIR_ENUM_IMPL(virFirewallBackend, > VIR_ENUM_DECL(virFirewallLayer); > VIR_ENUM_IMPL(virFirewallLayer, > VIR_FIREWALL_LAYER_LAST, > + "raw", > "ethernet", > "ipv4", > "ipv6", > @@ -54,6 +55,7 @@ typedef struct _virFirewallGroup virFirewallGroup; > VIR_ENUM_DECL(virFirewallLayerCommand); > VIR_ENUM_IMPL(virFirewallLayerCommand, > VIR_FIREWALL_LAYER_LAST, > + "", > EBTABLES, > IPTABLES, > IP6TABLES, > @@ -591,6 +593,7 @@ virFirewallCmdIptablesApply(virFirewall *firewall, > case VIR_FIREWALL_LAYER_IPV6: > virCommandAddArg(cmd, "-w"); > break; > + case VIR_FIREWALL_LAYER_RAW: > case VIR_FIREWALL_LAYER_LAST: > break; > } > @@ -672,43 +675,58 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, > size_t i; > int status; > > - cmd = virCommandNew(NFT); > + if (fwCmd->layer == VIR_FIREWALL_LAYER_RAW) { > > - if ((virFirewallTransactionGetFlags(firewall) & VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK) && > - fwCmd->argsLen > 1) { > - /* skip any leading options to get to command verb */ > - for (i = 0; i < fwCmd->argsLen - 1; i++) { > - if (fwCmd->args[i][0] != '-') > - break; > - } > + /* for VIR_FIREWALL_LAYER_RAW, args[0] is the binary name > + * and the rest are the args to that command > + */ > + cmd = virCommandNew(fwCmd->args[0]); > > - if (i + 1 < fwCmd->argsLen && > - VIR_NFTABLES_ARG_IS_CREATE(fwCmd->args[i])) { > + /* NB: RAW commands don't support auto-rollback command creation */ > > - cmdIdx = i; > - objectType = fwCmd->args[i + 1]; > + for (i = 1; i < fwCmd->argsLen; i++) > + virCommandAddArg(cmd, fwCmd->args[i]); > > - /* we currently only handle auto-rollback for rules, > - * chains, and tables, and those all can be "rolled > - * back" by a delete command using the handle that is > - * returned when "-ae" is added to the add/insert > - * command. > - */ > - if (STREQ_NULLABLE(objectType, "rule") || > - STREQ_NULLABLE(objectType, "chain") || > - STREQ_NULLABLE(objectType, "table")) { > + } else { > + > + cmd = virCommandNew(NFT); > + > + if ((virFirewallTransactionGetFlags(firewall) & VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK) && > + fwCmd->argsLen > 1) { > + /* skip any leading options to get to command verb */ > + for (i = 0; i < fwCmd->argsLen - 1; i++) { > + if (fwCmd->args[i][0] != '-') > + break; > + } > + > + if (i + 1 < fwCmd->argsLen && > + VIR_NFTABLES_ARG_IS_CREATE(fwCmd->args[i])) { > > - needRollback = true; > - /* this option to nft instructs it to add the > - * "handle" of the created object to stdout > + cmdIdx = i; > + objectType = fwCmd->args[i + 1]; > + > + /* we currently only handle auto-rollback for rules, > + * chains, and tables, and those all can be "rolled > + * back" by a delete command using the handle that is > + * returned when "-ae" is added to the add/insert > + * command. > */ > - virCommandAddArg(cmd, "-ae"); > + if (STREQ_NULLABLE(objectType, "rule") || > + STREQ_NULLABLE(objectType, "chain") || > + STREQ_NULLABLE(objectType, "table")) { > + > + needRollback = true; > + /* this option to nft instructs it to add the > + * "handle" of the created object to stdout > + */ > + virCommandAddArg(cmd, "-ae"); > + } > } > } > - } > > - for (i = 0; i < fwCmd->argsLen; i++) > - virCommandAddArg(cmd, fwCmd->args[i]); > + for (i = 0; i < fwCmd->argsLen; i++) > + virCommandAddArg(cmd, fwCmd->args[i]); > + } > > cmdStr = virCommandToString(cmd, false); > VIR_INFO("Applying '%s'", NULLSTR(cmdStr)); > diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h > index bce51259d2..636337e13e 100644 > --- a/src/util/virfirewall.h > +++ b/src/util/virfirewall.h > @@ -36,6 +36,7 @@ typedef struct _virFirewall virFirewall; > typedef struct _virFirewallCmd virFirewallCmd; > > typedef enum { > + VIR_FIREWALL_LAYER_RAW, > VIR_FIREWALL_LAYER_ETHERNET, > VIR_FIREWALL_LAYER_IPV4, > VIR_FIREWALL_LAYER_IPV6, > diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c > index 0a886780ad..21a9e02061 100644 > --- a/src/util/virfirewalld.c > +++ b/src/util/virfirewalld.c > @@ -43,6 +43,7 @@ VIR_LOG_INIT("util.firewalld"); > VIR_ENUM_DECL(virFirewallLayerFirewallD); > VIR_ENUM_IMPL(virFirewallLayerFirewallD, > VIR_FIREWALL_LAYER_LAST, > + "", > "eb", > "ipv4", > "ipv6", > -- > 2.47.0 > With regards, Daniel