[libvirt] [PATCH V2 1/2] Add support for firewalld (new version)
Laine Stump
laine at laine.org
Thu Aug 9 04:02:24 UTC 2012
On 08/08/2012 12:00 PM, Stefan Berger wrote:
> This patch is from Thomas Woerner's previous submission.
> I have split off the nwfilter part from this patch and fixed one compilation
> error caused by an unused variable.
>
> * bridge_driver, nwfilter_driver: new dbus filters to get FirewallD1.Reloaded
> signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1
> * iptables, ebtables, nwfilter_ebiptables_driver: use firewall-cmd direct
> passthrough interface
> * spec file changed as requested
> ---
> configure.ac | 8 +++++++
> libvirt.spec.in | 11 ++++++++++
> src/Makefile.am | 4 +--
> src/network/bridge_driver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++
> src/util/iptables.c | 19 +++++++++++++++++-
> 6 files changed, 120 insertions(+), 3 deletions(-)
>
> Index: libvirt-firewalld/configure.ac
> ===================================================================
> --- libvirt-firewalld.orig/configure.ac
> +++ libvirt-firewalld/configure.ac
> @@ -1282,6 +1282,14 @@ AM_CONDITIONAL([HAVE_POLKIT1], [test "x$
> AC_SUBST([POLKIT_CFLAGS])
> AC_SUBST([POLKIT_LIBS])
>
> +dnl firewalld
> +AC_ARG_WITH([firewalld],
> + AC_HELP_STRING([--with-firewalld], [enable firewalld support]))
> +if test "x$with_firewalld" = "xyes" ; then
> + AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld support is enabled])
> +fi
> +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" = "xyes"])
> +
> dnl Avahi library
> AC_ARG_WITH([avahi],
> AC_HELP_STRING([--with-avahi], [use avahi to advertise remote daemon @<:@default=check@:>@]),
> Index: libvirt-firewalld/libvirt.spec.in
> ===================================================================
> --- libvirt-firewalld.orig/libvirt.spec.in
> +++ libvirt-firewalld/libvirt.spec.in
> @@ -106,6 +106,7 @@
> %define with_sanlock 0%{!?_without_sanlock:0}
> %define with_systemd 0%{!?_without_systemd:0}
> %define with_numad 0%{!?_without_numad:0}
> +%define with_firewalld 0%{!?_without_firewalld:0}
>
> # Non-server/HV driver defaults which are always enabled
> %define with_python 0%{!?_without_python:1}
> @@ -146,6 +147,11 @@
> %define with_systemd 1
> %endif
>
> +# Fedora 18 / RHEL-7 are first where firewalld support is enabled
> +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
The comment and if statement don't match. Since firewalld is available
on F17, and official Fedora 17 builds will never get this specfile, we
could leave it at 17 in the if statement. I don't really have an
opinion, but the two lines should match.
Also, there is no Requires: line. But I guess that's intentional (and
rightly so) because we check for firewalld at run time and use it if
it's available, but go back to iptables/ebtables if it's not.
> +%define with_firewalld 1
> +%endif
> +
> # RHEL-5 has restricted QEMU to x86_64 only and is too old for LXC
> %if 0%{?rhel} == 5
> %define with_qemu_tcg 0
> @@ -1182,6 +1188,10 @@ of recent versions of Linux (and other O
> %define _without_driver_modules --without-driver-modules
> %endif
>
> +%if %{with_firewalld}
> +%define _with_firewalld --with-firewalld
> +%endif
> +
> %define when %(date +"%%F-%%T")
> %define where %(hostname)
> %define who %{?packager}%{!?packager:Unknown}
> @@ -1240,6 +1250,7 @@ autoreconf -if
> %{?_without_audit} \
> %{?_without_dtrace} \
> %{?_without_driver_modules} \
> + %{?_with_firewalld} \
The default of every other option (except rhel5-api) is "with". Is there
a reasonable way to make that the default as well, so that its logic
matches with the logic of all the other options? Or does that complicate
configure.ac too much?
> %{with_packager} \
> %{with_packager_version} \
> --with-qemu-user=%{qemu_user} \
> Index: libvirt-firewalld/src/Makefile.am
> ===================================================================
> --- libvirt-firewalld.orig/src/Makefile.am
> +++ libvirt-firewalld/src/Makefile.am
> @@ -989,7 +989,7 @@ libvirt_driver_network_la_SOURCES =
> libvirt_driver_network_la_LIBADD = libvirt_driver_network_impl.la
> if WITH_DRIVER_MODULES
> mod_LTLIBRARIES += libvirt_driver_network.la
> -libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la $(LIBNL_LIBS)
> +libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la $(LIBNL_LIBS) $(DBUS_LIBS)
> libvirt_driver_network_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
> else
> noinst_LTLIBRARIES += libvirt_driver_network.la
> @@ -999,7 +999,7 @@ endif
>
> libvirt_driver_network_impl_la_CFLAGS = \
> $(LIBNL_CFLAGS) \
> - -I$(top_srcdir)/src/conf $(AM_CFLAGS)
> + -I$(top_srcdir)/src/conf $(AM_CFLAGS) $(DBUS_CFLAGS)
> libvirt_driver_network_impl_la_SOURCES = $(NETWORK_DRIVER_SOURCES)
> endif
> EXTRA_DIST += network/default.xml
> Index: libvirt-firewalld/src/network/bridge_driver.c
> ===================================================================
> --- libvirt-firewalld.orig/src/network/bridge_driver.c
> +++ libvirt-firewalld/src/network/bridge_driver.c
> @@ -61,6 +61,7 @@
> #include "virnetdev.h"
> #include "virnetdevbridge.h"
> #include "virnetdevtap.h"
> +#include "virdbus.h"
>
> #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
> #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
> @@ -248,6 +249,24 @@ networkAutostartConfigs(struct network_d
> }
> }
>
> +#if HAVE_FIREWALLD
> +static DBusHandlerResult
> +firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED, DBusMessage *message, void *user_data) {
> + struct network_driver *_driverState = user_data;
> +
> + if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS,
> + "NameOwnerChanged") ||
> + dbus_message_is_signal(message, "org.fedoraproject.FirewallD1",
> + "Reloaded"))
> + {
The presence of this code means that if firewalld is enable, the
specfile MUST require dbus to be enabled as well (either that or the
firewalld support must be able to work without dbus, and this code must
depend on HAVE_DBUS in addition to HAVE_FIREWALLD).
> + VIR_DEBUG("Reload in bridge_driver because of firewalld.");
> + networkReloadIptablesRules(_driverState);
> + }
> +
> + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +}
> +#endif
> +
> /**
> * networkStartup:
> *
> @@ -256,6 +275,9 @@ networkAutostartConfigs(struct network_d
> static int
> networkStartup(int privileged) {
> char *base = NULL;
> +#ifdef HAVE_FIREWALLD
> + DBusConnection *sysbus = NULL;
> +#endif
>
> if (VIR_ALLOC(driverState) < 0)
> goto error;
> @@ -322,6 +344,30 @@ networkStartup(int privileged) {
>
> networkDriverUnlock(driverState);
>
> +#ifdef HAVE_FIREWALLD
> + if (!(sysbus = virDBusGetSystemBus())) {
> + virErrorPtr err = virGetLastError();
> + VIR_WARN("DBus not available, disabling firewalld support in bridge_driver: %s", err->message);
> + } else {
> + /* add matches for
> + * NameOwnerChanged on org.freedesktop.DBus for firewalld start/stop
> + * Reloaded on org.fedoraproject.FirewallD1 for firewalld reload
> + */
> + dbus_bus_add_match(sysbus,
> + "type='signal'"
> + ",interface='"DBUS_INTERFACE_DBUS"'"
> + ",member='NameOwnerChanged'"
> + ",arg0='org.fedoraproject.FirewallD1'",
> + NULL);
> + dbus_bus_add_match(sysbus,
> + "type='signal'"
> + ",interface='org.fedoraproject.FirewallD1'"
> + ",member='Reloaded'",
> + NULL);
> + dbus_connection_add_filter(sysbus, firewalld_dbus_filter_bridge, driverState, NULL);
> + }
> +#endif
> +
Same command about HAVE_DBUS, etc.
> return 0;
>
> out_of_memory:
> Index: libvirt-firewalld/src/util/ebtables.c
> ===================================================================
> --- libvirt-firewalld.orig/src/util/ebtables.c
> +++ libvirt-firewalld/src/util/ebtables.c
> @@ -176,11 +176,34 @@ ebtablesAddRemoveRule(ebtRules *rules, i
> const char *s;
> int n, command_idx;
>
> +#if HAVE_FIREWALLD
> + int ret;
> + char *firewall_cmd_path = NULL;
> + virCommandPtr cmd = NULL;
> +
> + firewall_cmd_path = virFindFileInPath("firewall-cmd");
> + if (firewall_cmd_path) {
> + cmd = virCommandNew(firewall_cmd_path);
> + virCommandAddArgList(cmd, "--state", NULL);
> + ret = virCommandRun(cmd, NULL);
> + if (ret != 0) {
> + VIR_FREE(firewall_cmd_path);
> + firewall_cmd_path = NULL;
> + }
> + virCommandFree(cmd);
> + }
> +#endif
> +
The piece above shouldn't be run each time an ebtable command is needed.
It should instead happen once when libvirtd is loaded, and cached.
The rest of this function really should be converted to use
virCommandRun instead of virRun. It could be done after this patch, but
I'd prefer it be done prior. (that's just a personal preference though -
shouldn't keep the patch from going in).
> n = 1 + /* /sbin/ebtables */
> 2 + /* --table foo */
> 2 + /* --insert bar */
> 1; /* arg */
>
> +#if HAVE_FIREWALLD
> + if (firewall_cmd_path)
> + n += 3; /* --direct --passthrough eb */
> +#endif
> +
> va_start(args, arg);
> while (va_arg(args, const char *))
> n++;
> @@ -192,6 +215,18 @@ ebtablesAddRemoveRule(ebtRules *rules, i
>
> n = 0;
>
> +#if HAVE_FIREWALLD
> + if (firewall_cmd_path) {
> + if (!(argv[n++] = strdup(firewall_cmd_path)))
> + goto error;
> + if (!(argv[n++] = strdup("--direct")))
> + goto error;
> + if (!(argv[n++] = strdup("--passthrough")))
> + goto error;
> + if (!(argv[n++] = strdup("eb")))
> + goto error;
> + } else
> +#endif
> if (!(argv[n++] = strdup(EBTABLES_PATH)))
> goto error;
>
> Index: libvirt-firewalld/src/util/iptables.c
> ===================================================================
> --- libvirt-firewalld.orig/src/util/iptables.c
> +++ libvirt-firewalld/src/util/iptables.c
> @@ -101,9 +101,26 @@ iptablesAddRemoveRule(iptRules *rules, i
> {
> va_list args;
> int ret;
> - virCommandPtr cmd;
> + virCommandPtr cmd = NULL;
> const char *s;
> +#if HAVE_FIREWALLD
> + char *firewall_cmd_path = NULL;
>
> + firewall_cmd_path = virFindFileInPath("firewall-cmd");
> + if (firewall_cmd_path) {
> + cmd = virCommandNew(firewall_cmd_path);
> + virCommandAddArgList(cmd, "--state", NULL);
> + ret = virCommandRun(cmd, NULL);
> + if (ret == 0) {
> + cmd = virCommandNew(firewall_cmd_path);
> + virCommandAddArgList(cmd, "--direct", "--passthrough",
> + (family == AF_INET6) ? "ipv6" : "ipv4", NULL);
> + } else
> + cmd = NULL;
> + VIR_FREE(firewall_cmd_path);
> + }
> + if (!cmd)
> +#endif
Same comment here - this should only happen once. (Maybe using the
fabled "virOnceInit() API that danpb mentioned in the comments of his
virObject series :-)
> cmd = virCommandNew((family == AF_INET6)
> ? IP6TABLES_PATH : IPTABLES_PATH);
>
More information about the libvir-list
mailing list