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

On 11/25/24 12:15 PM, Daniel P. Berrangé wrote: > On Mon, Nov 25, 2024 at 11:56:31AM -0500, Laine Stump wrote: > > On 11/25/24 5:44 AM, Daniel P. Berrangé wrote: > > > 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. > > > > I thought about that, but while layer is used to figure out the binary name > > for the iptables backend (because the different layers use ebtables, > > iptables, or ip6tables), in the case of the nftables backend all of the > > different layers use "nft" as the binary, and the layer indicates changes in > > a few of the arguments to that command (and really both your suggestion and > > mine are technically messed up, since the layer in the case of this > > checksum-fix filter should really be "ipv4"). > > Maybe we just shouldn't be pretending this is a firewall command at > all ? > > Even with iptables, this really isn't anything to do with traffic > filtering. Well, if you're going to be pedantic and say that the only things that are a part of the "firewall" are those bits that control whether or not the traffic passes, and *not* the bits that modify packets on their way through, then none of the rules that setup NAT should be a part of the firewall either. > iptables just happened to be a convenient place to put > the logic in the kernel at the time. > > 'tc' is the new "convenient" place to put the logic today. How about > putting a virNetDevFixDHCPChecksum() in virnetdev.h/c ? > > and just invoking this API after we've setup nftables rules ? That's kind of where I started out with this (having the tc command run not even as a part of networkAddFirewallRules(), but at the same level), but