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

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(a)redhat.com&gt; >> --- >> 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.