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@xxxxxxxxxx>
>
> [...]
> >
> >+
> > # 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list