Hello Laine, On 08/16/2012 08:18 AM, Laine Stump wrote: > From: Thomas Woerner <twoerner(a)redhat.com&gt; > > (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(a)gmail.com&gt; > Asad Saeed <asad.saeed(a)acidseed.com&gt; > Sukadev Bhattiprolu <sukadev(a)linux.vnet.ibm.com&gt; > - > + Thomas Woerner <twoerner(a)redhat.com&gt; > [....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.