Re: [libvirt] [PATCH 11/26] Introduce an object for managing firewall rulesets
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(a)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