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); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list