[libvirt] [PATCH 11/26] Introduce an object for managing firewall rulesets

[Jeff E. Nelson]

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