On 08/21/2012 11:24 AM, Thomas Woerner wrote:
> Hello Laine,
>
> On 08/16/2012 08:18 AM, Laine Stump wrote:
>> From: Thomas Woerner <twoerner@xxxxxxxxxx>
>>
>> (This is Thomas v3 version of 1/2 of the firewalld patches, modified
>> to check for firewall-cmd and firewalld state only once, rather than
>> every time an iptables rule is added or removed. It's not intended to
>> be pushed, because I'm still having issues with it, at least on my
>> machine. I'm mostly concerned with item (1) on the list below; the
>> others could be solved later or tolerated.)
>>
>> * configure.ac, spec file: firewalld defaults to enabled if dbus is
>> available, otherwise is disabled. If --with_firewalld is explicitly
>> requested and dbus is not available, configure will fail.
>>
>> * bridge_driver: add dbus filters to get the FirewallD1.Reloaded
>> signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1.
>> When these are encountered, reload all the iptables reuls of all
>> libvirt's virtual networks (similar to what happens when libvirtd is
>> restarted).
>>
>> * iptables, ebtables: use firewall-cmd's direct passthrough interface
>> when available, otherwise use iptables and ebtables commands. This
>> decision is made once the first time libvirt calls
>> iptables/ebtables, and that decision is maintained for the life of
>> libvirtd.
>>
>> * Note that the nwfilter part of this patch was separated out into
>> another patch by Stefan in V2, so that needs to be revised and
>> re-reviewed as well.
>>
>> ================
>>
>> All the configure.ac and specfile changes are unchanged from Thomas'
>> V3.
>>
>> V3 re-ran "firewall-cmd --state" every time a new rule was added,
>> which was extremely inefficient. V4 uses VIR_ONCE_GLOBAL_INIT to set
>> up a one-time initialization function.
>>
>> The VIR_ONCE_GLOBAL_INIT(x) macro references a static function called
>> vir(Ip|Eb)OnceInit(), which will then be called the first time that
>> the static function vir(Ip|Eb)TablesInitialize() is called (that
>> function is defined for you by the macro). This is
>> thread-safe, so there is no chance of any race.
>>
>> IMPORTANT NOTE: I've left the VIR_DEBUG messages in these two init
>> functions (one for iptables, on for ebtables) as VIR_WARN so that I
>> don't have to turn on all the other debug message just to see
>> these. Even if this patch doesn't need any other modification, those
>> messages need to be changed to VIR_DEBUG before pushing.
>>
>> This one-time initialization works well. However, I've encountered
>> problems with testing:
>>
>> 1) Whenever I have enabled the firewalld service, *all* attempts to
>> call firewall-cmd from within libvirtd end with firewall-cmd hanging
>> internally somewhere. This is *not* the case if firewall-cmd returns
>> non-0 in response to "firewall-cmd --state" (i.e. *that* command runs
>> and returns to libvirt successfully.)
>>
>> 2) If I start libvirtd while firewalld is stopped, then start
>> firewalld later, this triggers libvirtd to reload its iptables rules,
>> however it also spits out a *ton* of complaints about deletion failing
>> (I suppose because firewalld has nuked all of libvirt's rules). I
>> guess we need to suppress those messages (which is a more annoying
>> problem to fix than you might think, but that's another story).
>>
>> 3) I noticed a few times during this long line of errors that
>> firewalld made a complaint about "Resource Temporarily
>> unavailable. Having libvirtd access iptables commands directly at the
>> same time as firewalld is doing so is apparently problematic.
>>
>> 4) In general, I'm concerned about the "set it once and never change
>> it" method - if firewalld is disabled at libvirtd startup, causing
>> libvirtd to always use iptables/ebtables directly, this won't cause
>> *terrible* problems, but if libvirtd decides to use firewall-cmd and
>> firewalld is later disabled, libvirtd will not be able to recover.
>> ---
>> AUTHORS | 2 +-
>> configure.ac | 17 +++++++++++++++
>> libvirt.spec.in | 11 ++++++++++
>> src/Makefile.am | 4 ++--
>> src/network/bridge_driver.c | 49
>> ++++++++++++++++++++++++++++++++++++++++++
>> src/util/ebtables.c | 52
>> ++++++++++++++++++++++++++++++++++++++++++++-
>> src/util/iptables.c | 49
>> +++++++++++++++++++++++++++++++++++++++---
>> 7 files changed, 177 insertions(+), 7 deletions(-)
>>
>> diff --git a/AUTHORS b/AUTHORS
>> index 8581aea..5dec3a2 100644
>> --- a/AUTHORS
>> +++ b/AUTHORS
>> @@ -257,7 +257,7 @@ Patches have also been contributed by:
>> Frido Roose <frido.roose@xxxxxxxxx>
>> Asad Saeed <asad.saeed@xxxxxxxxxxxx>
>> Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx>
>> -
>> + Thomas Woerner <twoerner@xxxxxxxxxx>
>> [....send patches to get your name here....]
>>
>> The libvirt Logo was designed by Diana Fong
>> diff --git a/configure.ac b/configure.ac
>> index ba5a3cd..0150f99 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1321,6 +1321,22 @@ AM_CONDITIONAL([HAVE_POLKIT1], [test
>> "x$with_polkit1" = "xyes"])
>> AC_SUBST([POLKIT_CFLAGS])
>> AC_SUBST([POLKIT_LIBS])
>>
>> +dnl firewalld
>> +AC_ARG_WITH([firewalld],
>> + AC_HELP_STRING([--with-firewalld], [enable firewalld support
>> @<:@default=check@:>@]),
>> + [],
>> + [with_firewalld=check])
>> +if test "x$with_firewalld" = "xcheck" ; then
>> + with_firewalld=$with_dbus
>> +fi
>> +if test "x$with_firewalld" == "xyes" ; then
>> + if test "x$with_dbus" != "xyes" ; then
>> + AC_MSG_ERROR([You must have dbus enabled for firewalld support])
>> + fi
>> + AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld
>> support is enabled])
>> +fi
>> +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" != "xno"])
>> +
>> dnl Avahi library
>> AC_ARG_WITH([avahi],
>> AC_HELP_STRING([--with-avahi], [use avahi to advertise remote
>> daemon @<:@default=check@:>@]),
>> @@ -3028,6 +3044,7 @@ AC_MSG_NOTICE([ sanlock: $SANLOCK_CFLAGS
>> $SANLOCK_LIBS])
>> else
>> AC_MSG_NOTICE([ sanlock: no])
>> fi
>> +AC_MSG_NOTICE([firewalld: $with_firewalld])
>> if test "$with_avahi" = "yes" ; then
>> AC_MSG_NOTICE([ avahi: $AVAHI_CFLAGS $AVAHI_LIBS])
>> else
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 67b955a..ea2fd88 100644
>> --- a/libvirt.spec.in
>> +++ b/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
>> +%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 OSes).
>> %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} \
>> %{with_packager} \
>> %{with_packager_version} \
>> --with-qemu-user=%{qemu_user} \
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index b5f8056..6a94ecc 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -988,7 +988,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
>> @@ -998,7 +998,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
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 474bbfa..e3f0c1c 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -62,6 +62,7 @@
>> #include "virnetdevbridge.h"
>> #include "virnetdevtap.h"
>> #include "virnetdevvportprofile.h"
>> +#include "virdbus.h"
>>
>> #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
>> #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
>> @@ -249,6 +250,25 @@ networkAutostartConfigs(struct network_driver
>> *driver) {
>> }
>> }
>>
>> +#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"))
>> + {
>> + VIR_DEBUG("Reload in bridge_driver because of firewalld.");
>> + networkReloadIptablesRules(_driverState);
>> + }
>> +
>> + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>> +}
>> +#endif
>> +
>> /**
>> * networkStartup:
>> *
>> @@ -257,6 +277,9 @@ networkAutostartConfigs(struct network_driver
>> *driver) {
>> static int
>> networkStartup(int privileged) {
>> char *base = NULL;
>> +#ifdef HAVE_FIREWALLD
>> + DBusConnection *sysbus = NULL;
>> +#endif
>>
>> if (VIR_ALLOC(driverState) < 0)
>> goto error;
>> @@ -323,6 +346,32 @@ 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
>> +
>> return 0;
>>
>> out_of_memory:
>> diff --git a/src/util/ebtables.c b/src/util/ebtables.c
>> index ca056b1..1a78f89 100644
>> --- a/src/util/ebtables.c
>> +++ b/src/util/ebtables.c
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (C) 2007-2010 Red Hat, Inc.
>> + * Copyright (C) 2007-2012 Red Hat, Inc.
>> * Copyright (C) 2009 IBM Corp.
>> *
>> * This library is free software; you can redistribute it and/or
>> @@ -45,6 +45,38 @@
>> #include "memory.h"
>> #include "virterror_internal.h"
>> #include "logging.h"
>> +#include "threads.h"
>> +
>> +#if HAVE_FIREWALLD
>> +static char *firewall_cmd_path = NULL;
>> +
>> +static int
>> +virEbTablesOnceInit(void)
>> +{
>> + firewall_cmd_path = virFindFileInPath("firewall-cmd");
>> + if (!firewall_cmd_path) {
>> + VIR_WARN("firewall-cmd not found on system. "
>> + "firewalld support disabled for ebtables.");
>> + } else {
>> + virCommandPtr cmd = virCommandNew(firewall_cmd_path);
>> + int status;
>> +
>> + virCommandAddArgList(cmd, "--state", NULL);
>> + if (virCommandRun(cmd, &status) < 0 || status != 0) {
>> + VIR_WARN("firewall-cmd found but disabled for ebtables");
>> + VIR_FREE(firewall_cmd_path);
>> + firewall_cmd_path = NULL;
>> + } else {
>> + VIR_WARN("using firewalld for ebtables commands");
>> + }
>> + virCommandFree(cmd);
>> + }
>> + return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virEbTables)
>> +
>> +#endif
>>
>> struct _ebtablesContext
>> {
>> @@ -181,6 +213,12 @@ ebtablesAddRemoveRule(ebtRules *rules, int
>> action, const char *arg, ...)
>> 2 + /* --insert bar */
>> 1; /* arg */
>>
>> +#if HAVE_FIREWALLD
>> + virEbTablesInitialize();
>> + if (firewall_cmd_path)
>> + n += 3; /* --direct --passthrough eb */
>> +#endif
>> +
>> va_start(args, arg);
>> while (va_arg(args, const char *))
>> n++;
>> @@ -192,6 +230,18 @@ ebtablesAddRemoveRule(ebtRules *rules, int
>> action, const char *arg, ...)
>>
>> 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;
>>
>> diff --git a/src/util/iptables.c b/src/util/iptables.c
>> index b23aca9..d8fdd3b 100644
>> --- a/src/util/iptables.c
>> +++ b/src/util/iptables.c
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (C) 2007-2011 Red Hat, Inc.
>> + * Copyright (C) 2007-2012 Red Hat, Inc.
>> *
>> * This library is free software; you can redistribute it and/or
>> * modify it under the terms of the GNU Lesser General Public
>> @@ -43,6 +43,38 @@
>> #include "memory.h"
>> #include "virterror_internal.h"
>> #include "logging.h"
>> +#include "threads.h"
>> +
>> +#if HAVE_FIREWALLD
>> +static char *firewall_cmd_path = NULL;
>> +
>> +static int
>> +virIpTablesOnceInit(void)
>> +{
>> + firewall_cmd_path = virFindFileInPath("firewall-cmd");
>> + if (!firewall_cmd_path) {
>> + VIR_WARN("firewall-cmd not found on system. "
>> + "firewalld support disabled for iptables.");
>> + } else {
>> + virCommandPtr cmd = virCommandNew(firewall_cmd_path);
>> + int status;
>> +
>> + virCommandAddArgList(cmd, "--state", NULL);
>> + if (virCommandRun(cmd, &status) < 0 || status != 0) {
>> + VIR_WARN("firewall-cmd found but disabled for iptables");
>> + VIR_FREE(firewall_cmd_path);
>> + firewall_cmd_path = NULL;
>> + } else {
>> + VIR_WARN("using firewalld for iptables commands");
>> + }
>> + virCommandFree(cmd);
>> + }
>> + return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virIpTables)
>> +
>> +#endif
>>
>> #define VIR_FROM_THIS VIR_FROM_NONE
>>
>> @@ -101,11 +133,22 @@ iptablesAddRemoveRule(iptRules *rules, int
>> family, int action,
>> {
>> va_list args;
>> int ret;
>> - virCommandPtr cmd;
>> + virCommandPtr cmd = NULL;
>> const char *s;
>>
>> - cmd = virCommandNew((family == AF_INET6)
>> +#if HAVE_FIREWALLD
>> + virIpTablesInitialize();
>> + if (firewall_cmd_path) {
>> + cmd = virCommandNew(firewall_cmd_path);
>> + virCommandAddArgList(cmd, "--direct", "--passthrough",
>> + (family == AF_INET6) ? "ipv6" : "ipv4",
>> NULL);
>> + }
>> +#endif
>> +
>> + if (cmd == NULL) {
>> + cmd = virCommandNew((family == AF_INET6)
>> ? IP6TABLES_PATH : IPTABLES_PATH);
>> + }
>>
>> virCommandAddArgList(cmd, "--table", rules->table,
>> action == ADD ? "--insert" : "--delete",
>>
>
> here is my ACK for the changes.
Thanks. I just pushed it (along with a slightly tweaked version of
Stefan's nwfilter+firewalld patch).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list