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

[Laine Stump]

On 5/3/23 12:05 PM, Daniel P. Berrangé wrote:
> 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 at redhat.com>
>>> ---
>>>   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

Well done! You've caught the bit of the code that I felt the most 
uncomfortable about and mapped it out in a way that I can no longer 
gloss it over :-).


> 
> 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.

Well, there is more difference between virIptabledApplyFirewallRule() 
and virNftablesApplyFirewallRule() by the time you get to the end of the 
series - patches 20/28 and 21/28 add in the code that automatically 
generates a rollback rule (the command needed to remove the rule that is 
being added).

I haven't fully considered your comments on 18/28 yet, but it sounds 
like you think we don't need to automatically generate these rules, 
which would make for less difference between the backends. I still don't 
like the idea of *not* auto-generating rollback/removal rules when 
they're needed. Maybe the circular dependency could be eliminated by 
passing virFirewallApplRule a function that should be called to generate 
the rollback rule; this pointer would be NULL in the cases that a 
removal rule was unnecessary. (I'll try to avoid anything like that - 
simpler is better)