[libvirt] [PATCH] * configura.ac, spec file: firewalld now defaults to enabled, depends on dbus * fixed comment for with_firewalld define * 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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120813/7991df64/attachment-0001.sig>