On Tue, Apr 15, 2014 at 05:15:02PM -0400, Stefan Berger wrote:
On 04/08/2014 11:38 AM, Daniel P. Berrange wrote:
The network and nwfilter drivers both have a need to update firewall rules. The currently share no code for interacting with iptables / firewalld. The nwfilter driver is fairly tied to the concept of creating shell scripts to execute which makes it very hard to port to talk to firewalld via DBus APIs.
This patch introduces a virFirewallPtr object which is able to represent a complete sequence of rule changes, with the ability to have multiple transactional checkpoints with rollbacks. By formally separating the definition of the rules to be applied from the mechanism used to apply them, it is also possible to write a firewall engine that uses firewalld DBus APIs natively instead of via the slow firewalld-cmd.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
[...]
+ # util/virhash.h virHashAddEntry; virHashCreate; diff --git a/src/util/virerror.c b/src/util/virerror.c index cbbaa83..e0bc970 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -129,6 +129,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Systemd", "Bhyve", "Crypto", + "Firewall", )
[ ... ]
+#define VIR_FIREWALL_RETURN_IF_ERROR(firewall) \ + if (!firewall || firewall->err) \ + return; +
I have a strong prejudice *against* writing statements in a macro *unless* the macro expands as a SINGLE statement under all possible usage scenarios; otherwise the macro is just a latent problem waiting to happen. Consider what happens if I use the macro like this: 1: if (condition) 2: VIR_FIREWALL_RETURN_IF_ERROR(fw) 3: else 4: DO_SOMETHING_ELSE(fw); # test some other condition I don't get what I expected; the =else= on line 3 matches with the =if= hidden in the macro on line 2, not with the =if= on line 1! Please *consider* defining the macro like this: #define VIR_FIREWALL_RETURN_IF_ERROR(firewall) \ do { \ if (!firewall || firewall->err) \ return; \ } while (0) The "do { ... } while (0)" is a single statement that is guaranteed to execute exactly once. Modern compilers will notice and get rid of the loop, so there is no overhead. Also note the trailing ";" is omitted. This goes back to a time when compilers would complain about two semicolons in a row with no intervening statement, but I don't think that is common these days.
ACK with nits addressed.
Ditto. -Jeff