On Mon, Nov 25, 2024 at 06:21:15PM -0500, Laine Stump wrote: > 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 ...snip... Ok, since its all messy, I'll defer to your preference :-) 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 :|
