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@xxxxxxxxxx>
---
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)
