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.
Since Stefan is away, and we would like to get this into libvirt sooner
rather than later, I took another look through it, fixed two nits, and
pushed it.
>
> ---
> 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
> +
> +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();
> + 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
> + 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;
I removed the above NULL initializations, because virAsPrintf takes care
of that for us.
> +
> + 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();
> 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;
> +
> +
This is unnecessary (since that function is only called from another
nwfilter source compiled into the same module), but actually causes an
error in make check, because the symbol can't be found in libvirt.so.
I removed these lines from libvirt_private.syms.
> # 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__ */
>
ACK with those nits fixed. As I said above, I've fixed the nits and
pushed the patch (along with a newer version of 1/2).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
