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);

On 08/13/2012 01:23 PM, Thomas Woerner wrote: Subject line was a mess. You forgot a one-line summary followed by a blank line before the actual listing of changes you made. Also, by replying in a thread, it's not clear whether this is a new patch unrelated to the rest of the thread, or a resubmission. Using 'git send-email --subject-prefix=PATCHv2' will help on that front. Can you please submit a new version with fixed subject, and perhaps as a standalone thread to make it easier to spot? Also, these findings need fixing:

--- configure.ac | 11 +++++++++++ libvirt.spec.in | 11 +++++++++++ src/Makefile.am | 4 ++-- src/network/bridge_driver.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ src/util/ebtables.c | 35 ++++++++++++++++++++++++++++++++++ src/util/iptables.c | 21 +++++++++++++++++++-- 6 files changed, 124 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac index 8a04d91..8fd3e34 100644 --- a/configure.ac +++ b/configure.ac @@ -1282,6 +1282,17 @@ 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]))

Needs a fourth argument to AC_ARG_WITH to set the default when neither '--with-firewalld' nor '--without-firewalld' is given.

+if test "x$with_firewalld" != "xno" ; then + AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld support is enabled]) + if test "x$with_dbus" != "xyes" ; then + AC_MSG_ERROR([You must have dbus enabled for firewalld support]) + fi +fi +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" != "xno"])

We tend to prefer tri-state checking (yes, no, and 'check', with the default being check, and where check changes to yes or no depending on finding prerequisites in place). Also, at the end of configure.ac, we have a series of outputs that summarize what was selected; firewalld should be listed in that summary.

+++ b/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_driver *driver) { } }

+#if HAVE_FIREWALLD +static DBusHandlerResult +firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED, DBusMessage *message, void *user_data) {

Long line. I'd wrap at the comma between 'connection ATTRIBUTE_UNUSED, DBusMessage'. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/08/2012 12:00 PM, Stefan Berger wrote:

This patch provides basic support for using firewalld's firewall-cmd rather than then plain eb/ip(6)tables commands.

--- src/Makefile.am | 4 src/conf/nwfilter_conf.h | 1 src/libvirt_private.syms | 4 src/nwfilter/nwfilter_driver.c | 172 ++++++++++++++++++++++++++++-- src/nwfilter/nwfilter_driver.h | 2 src/nwfilter/nwfilter_ebiptables_driver.c | 144 +++++++++++++++++++++---- 6 files changed, 292 insertions(+), 35 deletions(-)

Index: libvirt-firewalld/src/Makefile.am =================================================================== --- libvirt-firewalld.orig/src/Makefile.am +++ libvirt-firewalld/src/Makefile.am @@ -1149,9 +1149,9 @@ noinst_LTLIBRARIES += libvirt_driver_nwf #libvirt_la_BUILT_LIBADD += libvirt_driver_nwfilter.la endif libvirt_driver_nwfilter_la_CFLAGS = $(LIBPCAP_CFLAGS) \ - -I$(top_srcdir)/src/conf $(LIBNL_CFLAGS) $(AM_CFLAGS) + -I$(top_srcdir)/src/conf $(LIBNL_CFLAGS) $(AM_CFLAGS) $(DBUS_CFLAGS) libvirt_driver_nwfilter_la_LDFLAGS = $(LD_AMFLAGS) -libvirt_driver_nwfilter_la_LIBADD = $(LIBPCAP_LIBS) $(LIBNL_LIBS) +libvirt_driver_nwfilter_la_LIBADD = $(LIBPCAP_LIBS) $(LIBNL_LIBS) $(DBUS_LIBS) if WITH_DRIVER_MODULES libvirt_driver_nwfilter_la_LIBADD += ../gnulib/lib/libgnu.la libvirt_driver_nwfilter_la_LDFLAGS += -module -avoid-version Index: libvirt-firewalld/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-firewalld.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-firewalld/src/nwfilter/nwfilter_driver.c @@ -27,6 +27,9 @@

#include <config.h>

+#include "virdbus.h" +#include "logging.h" + #include "internal.h"

#include "virterror_internal.h" @@ -45,10 +48,24 @@

#define VIR_FROM_THIS VIR_FROM_NWFILTER

+#define DBUS_RULE_FWD_NAMEOWNERCHANGED \ + "type='signal'" \ + ",interface='"DBUS_INTERFACE_DBUS"'" \ + ",member='NameOwnerChanged'" \ + ",arg0='org.fedoraproject.FirewallD1'" + +#define DBUS_RULE_FWD_RELOADED \ + "type='signal'" \ + ",interface='org.fedoraproject.FirewallD1'" \ + ",member='Reloaded'" + + static virNWFilterDriverStatePtr driverState;

static int nwfilterDriverShutdown(void);

+static int nwfilterDriverReload(void); + static void nwfilterDriverLock(virNWFilterDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -58,6 +75,89 @@ static void nwfilterDriverUnlock(virNWFi virMutexUnlock(&driver->lock); }

+#if HAVE_FIREWALLD

Again, either firewalld needs to require WITH_DBUS, or this needs to be conditional on both HAVE_FIREWALLD and HAVE_DBUS.

+ +static DBusHandlerResult +nwfilterFirewalldDBusFilter(DBusConnection *connection ATTRIBUTE_UNUSED, + DBusMessage *message, + void *user_data ATTRIBUTE_UNUSED) +{ + if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS, + "NameOwnerChanged") || + dbus_message_is_signal(message, "org.fedoraproject.FirewallD1", + "Reloaded")) { + VIR_DEBUG("Reload in nwfilter_driver because of firewalld."); + nwfilterDriverReload(); + } + + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; +} + +static void +nwfilterDriverRemoveDBusMatches(void) +{ + DBusConnection *sysbus; + + sysbus = virDBusGetSystemBus(); + if (sysbus) { + dbus_bus_remove_match(sysbus, + DBUS_RULE_FWD_NAMEOWNERCHANGED, + NULL); + dbus_bus_remove_match(sysbus, + DBUS_RULE_FWD_RELOADED, + NULL); + dbus_connection_remove_filter(sysbus, nwfilterFirewalldDBusFilter, NULL); + } +} + +/** + * virNWFilterDriverInstallDBusMatches + * + * Startup DBus matches for monitoring the state of firewalld + */ +static int +nwfilterDriverInstallDBusMatches(DBusConnection *sysbus) +{ + int ret = 0; + + if (!sysbus) { + ret = -1; + } 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, + DBUS_RULE_FWD_NAMEOWNERCHANGED, + NULL); + dbus_bus_add_match(sysbus, + DBUS_RULE_FWD_RELOADED, + NULL); + if (!dbus_connection_add_filter(sysbus, nwfilterFirewalldDBusFilter, + NULL, NULL)) { + VIR_WARN(("Adding a filter to the DBus connection failed")); + nwfilterDriverRemoveDBusMatches(); + ret = -1; + } + } + + return ret; +} + +#else /* HAVE_FIREWALLD */ + +static void +nwfilterDriverRemoveDBusMatches(void) +{ +} + +static int +nwfilterDriverInstallDBusMatches(DBusConnection *sysbus ATTRIBUTE_UNUSED) +{ + return 0; +} + +#endif /* HAVE_FIREWALLD */

/** * virNWFilterStartup: @@ -65,14 +165,24 @@ static void nwfilterDriverUnlock(virNWFi * Initialization function for the QEmu daemon */ static int -nwfilterDriverStartup(int privileged) { +nwfilterDriverStartup(int privileged) +{ char *base = NULL; + DBusConnection *sysbus = virDBusGetSystemBus(); + + if (VIR_ALLOC(driverState) < 0) + goto alloc_err_exit; + + if (virMutexInit(&driverState->lock) < 0) + goto err_free_driverstate; + + driverState->watchingFirewallD = (sysbus != NULL);

if (!privileged) return 0;

if (virNWFilterIPAddrMapInit() < 0) - return -1; + goto err_free_driverstate; if (virNWFilterLearnInit() < 0) goto err_exit_ipaddrmapshutdown; if (virNWFilterDHCPSnoopInit() < 0) @@ -81,16 +191,26 @@ nwfilterDriverStartup(int privileged) { virNWFilterTechDriversInit(privileged);

if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB) < 0) - goto conf_init_err; - - if (VIR_ALLOC(driverState) < 0) - goto alloc_err_exit; - - if (virMutexInit(&driverState->lock) < 0) - goto alloc_err_exit; + goto err_techdrivers_shutdown;

nwfilterDriverLock(driverState);

+ /* + * startup the DBus late so we don't get a reload signal while + * initializing + */ + if (nwfilterDriverInstallDBusMatches(sysbus) < 0) { + VIR_ERROR(_("DBus matches could not be installed. Disabling nwfilter " + "driver")); + /* + * unfortunately this is fatal since virNWFilterTechDriversInit + * may have caused the ebiptables driver to use the firewall tool + * but now that the watches don't work, we just disable the nwfilter + * driver + */ + goto error; + } + if (privileged) { if ((base = strdup (SYSCONFDIR "/libvirt")) == NULL) goto out_of_memory; @@ -124,9 +244,11 @@ error: nwfilterDriverShutdown();

alloc_err_exit: - virNWFilterConfLayerShutdown();

There's a lot of reorganization here that's not really related to adding firewalld support, but seems more of a general refactoring. Would it be possible to split that out in to a separate prerequisite patch so it's easier to see exactly what's going in for the purpose of supporting firewalld (and also to make it possible to test the refactoring separate from adding firewalld support, if it ever comes to that)?

+ return -1;

-conf_init_err: + nwfilterDriverUnlock(driverState); + +err_techdrivers_shutdown: virNWFilterTechDriversShutdown(); virNWFilterDHCPSnoopShutdown(); err_exit_learnshutdown: @@ -134,6 +256,9 @@ err_exit_learnshutdown: err_exit_ipaddrmapshutdown: virNWFilterIPAddrMapShutdown();

+err_free_driverstate: + VIR_FREE(driverState); + return -1; }

@@ -192,6 +317,29 @@ nwfilterDriverActive(void) {

nwfilterDriverLock(driverState); ret = driverState->nwfilters.count ? 1 : 0; + ret |= driverState->watchingFirewallD; + nwfilterDriverUnlock(driverState); + + return ret; +} + +/** + * virNWFilterIsWatchingFirewallD: + * + * Checks if the nwfilter has the DBus watches for FirewallD installed. + * + * Returns true if it is watching firewalld, false otherwise + */ +bool +virNWFilterDriverIsWatchingFirewallD(void) +{ + bool ret; + + if (!driverState) + return false; + + nwfilterDriverLock(driverState); + ret = driverState->watchingFirewallD; nwfilterDriverUnlock(driverState);

return ret; @@ -215,6 +363,8 @@ nwfilterDriverShutdown(void) {

nwfilterDriverLock(driverState);

+ nwfilterDriverRemoveDBusMatches(); + /* free inactive nwfilters */ virNWFilterObjListFree(&driverState->nwfilters);

Index: libvirt-firewalld/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-firewalld.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-firewalld/src/nwfilter/nwfilter_ebiptables_driver.c @@ -36,6 +36,7 @@ #include "virterror_internal.h" #include "domain_conf.h" #include "nwfilter_conf.h" +#include "nwfilter_driver.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" #include "virfile.h" @@ -145,11 +146,11 @@ static const char ebiptables_script_set_ #define NWFILTER_FUNC_SET_IFS ebiptables_script_set_ifs

#define NWFILTER_SET_EBTABLES_SHELLVAR(BUFPTR) \ - virBufferAsprintf(BUFPTR, "EBT=%s\n", ebtables_cmd_path); + virBufferAsprintf(BUFPTR, "EBT=\"%s\"\n", ebtables_cmd_path); #define NWFILTER_SET_IPTABLES_SHELLVAR(BUFPTR) \ - virBufferAsprintf(BUFPTR, "IPT=%s\n", iptables_cmd_path); + virBufferAsprintf(BUFPTR, "IPT=\"%s\"\n", iptables_cmd_path); #define NWFILTER_SET_IP6TABLES_SHELLVAR(BUFPTR) \ - virBufferAsprintf(BUFPTR, "IPT=%s\n", ip6tables_cmd_path); + virBufferAsprintf(BUFPTR, "IPT=\"%s\"\n", ip6tables_cmd_path);

#define VIRT_IN_CHAIN "libvirt-in" #define VIRT_OUT_CHAIN "libvirt-out" @@ -4121,23 +4122,101 @@ virNWFilterTechDriver ebiptables_driver .removeBasicRules = ebtablesRemoveBasicRules, };

- +/* + * ebiptablesDriverInitWithFirewallD + * + * Try to use firewall-cmd by testing it once; if it works, have ebtables + * and ip6tables commands use firewall-cmd. + */ static int -ebiptablesDriverInit(bool privileged) +ebiptablesDriverInitWithFirewallD(void) { virBuffer buf = VIR_BUFFER_INITIALIZER; - char *errmsg = NULL; + char *firewall_cmd_path; + char *output = NULL; + int ret = -1;

- if (!privileged) - return 0; + if (!virNWFilterDriverIsWatchingFirewallD()) + return -1;

- if (virMutexInit(&execCLIMutex) < 0) - return -EINVAL; + firewall_cmd_path = virFindFileInPath("firewall-cmd");

- gawk_cmd_path = virFindFileInPath("gawk"); - grep_cmd_path = virFindFileInPath("grep"); + if (firewall_cmd_path) { + virBufferAsprintf(&buf, "FWC=%s\n", firewall_cmd_path); + virBufferAsprintf(&buf, + CMD_DEF("$FWC --state") CMD_SEPARATOR

I'm curious why you need to put the path to the command into a shell variable, and then reference the shell variable instead of just directly putting the path into the commandline.

+ CMD_EXEC + "%s", + CMD_STOPONERR(1)); + + if (ebiptablesExecCLI(&buf, NULL, &output) == 0 && + strlen(output) == 0) { + VIR_DEBUG("Using firewall-cmd in nwfilter_ebiptables_driver."); + ebtables_cmd_path = NULL; + iptables_cmd_path = NULL; + ip6tables_cmd_path = NULL;

It's not necessary to set these to NULL before calling virAsprintf - it ignores the initial value, and sets it to NULL on failure.

+ + ignore_value(virAsprintf(&ebtables_cmd_path, + "%s --direct --passthrough eb", + firewall_cmd_path)); + ignore_value(virAsprintf(&iptables_cmd_path, + "%s --direct --passthrough ipv4", + firewall_cmd_path)); + ignore_value(virAsprintf(&ip6tables_cmd_path, + "%s --direct --passthrough ipv6", + firewall_cmd_path)); + + if (!ebtables_cmd_path || !iptables_cmd_path || + !ip6tables_cmd_path) { + virReportOOMError(); + VIR_FREE(ebtables_cmd_path); + VIR_FREE(iptables_cmd_path); + VIR_FREE(ip6tables_cmd_path); + ret = -1; + goto err_exit; + } + ret = 0; + } + } + +err_exit: + VIR_FREE(firewall_cmd_path); + VIR_FREE(output); + + return ret; +}

+static int +ebiptablesDriverInitCLITools(void) +{ ebtables_cmd_path = virFindFileInPath("ebtables"); + if (!ebtables_cmd_path) + VIR_WARN("Could not find 'ebtables' executable"); + + iptables_cmd_path = virFindFileInPath("iptables"); + if (!iptables_cmd_path) + VIR_WARN("Could not find 'iptables' executable"); + + ip6tables_cmd_path = virFindFileInPath("ip6tables"); + if (!ip6tables_cmd_path) + VIR_WARN("Could not find 'ip6tables' executable"); + + return 0; +} + +/* + * ebiptablesDriverTestCLITools + * + * Test the CLI tools. If one is found not to be working, free the buffer + * holding its path as a sign that the tool cannot be used. + */ +static int +ebiptablesDriverTestCLITools(void) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *errmsg = NULL; + int ret = 0; + if (ebtables_cmd_path) { NWFILTER_SET_EBTABLES_SHELLVAR(&buf); /* basic probing */ @@ -4151,12 +4230,10 @@ ebiptablesDriverInit(bool privileged) VIR_FREE(ebtables_cmd_path); VIR_ERROR(_("Testing of ebtables command failed: %s"), errmsg); + ret = -1; } - } else { - VIR_WARN("Could not find 'ebtables' executable"); }

- iptables_cmd_path = virFindFileInPath("iptables"); if (iptables_cmd_path) { NWFILTER_SET_IPTABLES_SHELLVAR(&buf);

@@ -4170,12 +4247,10 @@ ebiptablesDriverInit(bool privileged) VIR_FREE(iptables_cmd_path); VIR_ERROR(_("Testing of iptables command failed: %s"), errmsg); + ret = -1; } - } else { - VIR_WARN("Could not find 'iptables' executable"); }

- ip6tables_cmd_path = virFindFileInPath("ip6tables"); if (ip6tables_cmd_path) { NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);

@@ -4189,11 +4264,38 @@ ebiptablesDriverInit(bool privileged) VIR_FREE(ip6tables_cmd_path); VIR_ERROR(_("Testing of ip6tables command failed: %s"), errmsg); + ret = -1; } - } else { - VIR_WARN("Could not find 'ip6tables' executable"); }

+ VIR_FREE(errmsg); + + return ret; +} + +static int +ebiptablesDriverInit(bool privileged) +{ + if (!privileged) + return 0; + + if (virMutexInit(&execCLIMutex) < 0) + return -EINVAL; + + gawk_cmd_path = virFindFileInPath("gawk"); + grep_cmd_path = virFindFileInPath("grep"); + + /* + * check whether we can run with firewalld's tools -- + * if not, we just fall back to eb/iptables command + * line tools. + */ + if (ebiptablesDriverInitWithFirewallD() < 0) + ebiptablesDriverInitCLITools(); + + /* make sure tools are available and work */ + ebiptablesDriverTestCLITools(); + /* ip(6)tables support needs gawk & grep, ebtables doesn't */ if ((iptables_cmd_path != NULL || ip6tables_cmd_path != NULL) && (!grep_cmd_path || !gawk_cmd_path)) { @@ -4203,8 +4305,6 @@ ebiptablesDriverInit(bool privileged) VIR_FREE(ip6tables_cmd_path); }

- VIR_FREE(errmsg); - if (!ebtables_cmd_path && !iptables_cmd_path && !ip6tables_cmd_path) { VIR_ERROR(_("firewall tools were not found or cannot be used")); ebiptablesDriverShutdown();

Just a note again at the bgottom of this file - it looks like there's a lot of general refactoring here, and that's obscuring what is being done specifically to support using firewalld's commandline rather than iptables/ebtables.

Index: libvirt-firewalld/src/conf/nwfilter_conf.h =================================================================== --- libvirt-firewalld.orig/src/conf/nwfilter_conf.h +++ libvirt-firewalld/src/conf/nwfilter_conf.h @@ -563,6 +563,7 @@ struct _virNWFilterDriverState { virNWFilterObjList nwfilters;

char *configDir; + bool watchingFirewallD; };

Index: libvirt-firewalld/src/libvirt_private.syms =================================================================== --- libvirt-firewalld.orig/src/libvirt_private.syms +++ libvirt-firewalld/src/libvirt_private.syms @@ -891,6 +891,10 @@ virNWFilterIPAddrMapInit; virNWFilterIPAddrMapShutdown;

+# nwfilter_driver.h +virNWFilterDriverIsWatchingFirewallD; + + # nwfilter_params.h virNWFilterHashTableCreate; virNWFilterHashTableFree; Index: libvirt-firewalld/src/nwfilter/nwfilter_driver.h =================================================================== --- libvirt-firewalld.orig/src/nwfilter/nwfilter_driver.h +++ libvirt-firewalld/src/nwfilter/nwfilter_driver.h @@ -33,4 +33,6 @@

int nwfilterRegister(void);

+bool virNWFilterDriverIsWatchingFirewallD(void); + #endif /* __VIR_NWFILTER_DRIVER_H__ */

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list