[libvirt] [PATCH 2/5] util: move all firewalld-specific stuff into its own file

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 09:57:34PM -0500, Laine Stump wrote:
> Since I'm going to be adding at least one more firewalld-specific
> function, this seems like a good time to separate the code that's
> unique to firewalld from the more-generic "firewall" file.
> 
> Signed-off-by: Laine Stump <laine at laine.org>
> ---
>  include/libvirt/virterror.h |   1 +
>  src/libvirt_private.syms    |   3 +
>  src/util/Makefile.inc.am    |   2 +
>  src/util/virerror.c         |   1 +
>  src/util/virfirewall.c      |  86 ++----------------------
>  src/util/virfirewalld.c     | 128 ++++++++++++++++++++++++++++++++++++
>  src/util/virfirewalld.h     |  33 ++++++++++
>  src/util/virfirewallpriv.h  |   2 -
>  tests/virfirewalltest.c     |   1 +
>  9 files changed, 173 insertions(+), 84 deletions(-)
>  create mode 100644 src/util/virfirewalld.c
>  create mode 100644 src/util/virfirewalld.h

> +    if (error.level == VIR_ERR_ERROR) {
> +        /*
> +         * As of firewalld-0.3.9.3-1.fc20.noarch the name and
> +         * message fields in the error look like
> +         *
> +         *    name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException"
> +         * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete
> +         *          INPUT --in-interface virbr0 --protocol udp --destination-port 53
> +         *          --jump ACCEPT' failed: iptables: Bad rule (does a matching rule
> +         *          exist in that chain?)."
> +         *
> +         * We'd like to only ignore DBus errors precisely related to the failure
> +         * of iptables/ebtables commands. A well designed DBus interface would
> +         * return specific named exceptions not the top level generic python dbus
> +         * exception name. With this current scheme our only option is todo a
> +         * sub-string match for 'COMMAND_FAILED' on the message. eg like
> +         *
> +         * if (ignoreErrors &&
> +         *     STREQ(error.name,
> +         *           "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") &&
> +         *     STRPREFIX(error.message, "COMMAND_FAILED"))
> +         *    ...
> +         *
> +         * But this risks our error detecting code being broken if firewalld changes
> +         * ever alter the message string, so we're avoiding doing that.
> +         */

We really ought to ask the firewalld guys to improve this since
we have good communication with them now :-)

> +        if (ignoreErrors) {
> +            VIR_DEBUG("Ignoring error '%s': '%s'",
> +                      error.str1, error.message);
> +        } else {
> +            virReportErrorObject(&error);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (virDBusMessageRead(reply, "s", output) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virResetError(&error);
> +    virDBusMessageUnref(reply);
> +    return ret;
> +}


> diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h
> new file mode 100644
> index 0000000000..c1c929399a
> --- /dev/null
> +++ b/src/util/virfirewalld.h
> @@ -0,0 +1,33 @@


> +#ifndef LIBVIRT_VIRFIREWALLD_H
> +# define LIBVIRT_VIRFIREWALLD_H
> +
> +# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"

This is only needed for tests so should go in a priv.h header
as it was before.

> +
> +int virFirewallDStatus(void);

Perhaps  virFirewallDIsRegistered() ?

Could you put API docs for this since it has an unusal tri-state
return value.

> +
> +int virFirewallDApplyRule(virFirewallLayer layer,
> +                          char **args, size_t argsLen,
> +                          bool ignoreErrors,
> +                          char **output);

Would be nice to have API docs for this too

> +
> +#endif /* LIBVIRT_VIRFIREWALLD_H */
> diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h
> index efa94a7da4..7c31d0680d 100644
> --- a/src/util/virfirewallpriv.h
> +++ b/src/util/virfirewallpriv.h
> @@ -27,8 +27,6 @@
>  
>  # include "virfirewall.h"
>  
> -# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"
> -
>  typedef enum {
>      VIR_FIREWALL_BACKEND_AUTOMATIC,
>      VIR_FIREWALL_BACKEND_DIRECT,
> diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
> index 63b9ced820..573ab1f9cd 100644
> --- a/tests/virfirewalltest.c
> +++ b/tests/virfirewalltest.c
> @@ -27,6 +27,7 @@
>  # include "vircommandpriv.h"
>  # define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW
>  # include "virfirewallpriv.h"
> +# include "virfirewalld.h"

Should be virfirewalldpriv.h


Assuming those small changes are made

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|