Since I'm going to be adding at least one more firewalld-specific
function, this seems like a good time to separate the code that's
unique to firewalld from the more-generic "firewall" file.

Signed-off-by: Laine Stump <laine@laine.org>
---
 include/libvirt/virterror.h |   1 +
 src/libvirt_private.syms    |   3 +
 src/util/Makefile.inc.am    |   2 +
 src/util/virerror.c         |   1 +
 src/util/virfirewall.c      |  86 ++----------------------
 src/util/virfirewalld.c     | 128 ++++++++++++++++++++++++++++++++++++
 src/util/virfirewalld.h     |  33 ++++++++++
 src/util/virfirewallpriv.h  |   2 -
 tests/virfirewalltest.c     |   1 +
 9 files changed, 173 insertions(+), 84 deletions(-)
 create mode 100644 src/util/virfirewalld.c
 create mode 100644 src/util/virfirewalld.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index fbbe2d5624..3c19ff5e2e 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -131,6 +131,7 @@ typedef enum {
     VIR_FROM_PERF = 65,         /* Error from perf */
     VIR_FROM_LIBSSH = 66,       /* Error from libssh connection transport */
     VIR_FROM_RESCTRL = 67,      /* Error from resource control */
+    VIR_FROM_FIREWALLD = 68,    /* Error from firewalld */
 
 # ifdef VIR_ENUM_SENTINELS
     VIR_ERR_DOMAIN_LAST
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c3d6306809..583868f422 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1918,6 +1918,9 @@ virFirewallSetLockOverride;
 virFirewallStartRollback;
 virFirewallStartTransaction;
 
+# util/virfirewalld.h
+virFirewallDApplyRule;
+virFirewallDStatus;
 
 # util/virfirmware.h
 virFirmwareFreeList;
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index 4295babac3..0295a1c7d0 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -64,6 +64,8 @@ UTIL_SOURCES = \
 	util/virfirewall.c \
 	util/virfirewall.h \
 	util/virfirewallpriv.h \
+	util/virfirewalld.c \
+	util/virfirewalld.h \
 	util/virfirmware.c \
 	util/virfirmware.h \
 	util/virgettext.c \
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 61b47d2be0..ae1efa72d8 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -138,6 +138,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
               "Perf", /* 65 */
               "Libssh transport layer",
               "Resource control",
+              "FirewallD",
               )
 
 
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 5a0cf95a44..d5d647fcc4 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -24,12 +24,12 @@
 
 #define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW
 #include "virfirewallpriv.h"
+#include "virfirewalld.h"
 #include "virerror.h"
 #include "virutil.h"
 #include "virstring.h"
 #include "vircommand.h"
 #include "virlog.h"
-#include "virdbus.h"
 #include "virfile.h"
 #include "virthread.h"
 
@@ -46,11 +46,6 @@ VIR_ENUM_IMPL(virFirewallLayerCommand, VIR_FIREWALL_LAYER_LAST,
               IPTABLES_PATH,
               IP6TABLES_PATH);
 
-VIR_ENUM_DECL(virFirewallLayerFirewallD)
-VIR_ENUM_IMPL(virFirewallLayerFirewallD, VIR_FIREWALL_LAYER_LAST,
-              "eb", "ipv4", "ipv6")
-
-
 struct _virFirewallRule {
     virFirewallLayer layer;
 
@@ -152,7 +147,7 @@ virFirewallValidateBackend(virFirewallBackend backend)
     VIR_DEBUG("Validating backend %d", backend);
     if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC ||
         backend == VIR_FIREWALL_BACKEND_FIREWALLD) {
-        int rv = virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE);
+        int rv = virFirewallDStatus();
 
         VIR_DEBUG("Firewalld is registered ? %d", rv);
         if (rv < 0) {
@@ -712,81 +707,8 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
                               bool ignoreErrors,
                               char **output)
 {
-    const char *ipv = virFirewallLayerFirewallDTypeToString(rule->layer);
-    DBusConnection *sysbus = virDBusGetSystemBus();
-    DBusMessage *reply = NULL;
-    virError error;
-    int ret = -1;
-
-    if (!sysbus)
-        return -1;
-
-    memset(&error, 0, sizeof(error));
-
-    if (!ipv) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Unknown firewall layer %d"),
-                       rule->layer);
-        goto cleanup;
-    }
-
-    if (virDBusCallMethod(sysbus,
-                          &reply,
-                          &error,
-                          VIR_FIREWALL_FIREWALLD_SERVICE,
-                          "/org/fedoraproject/FirewallD1",
-                          "org.fedoraproject.FirewallD1.direct",
-                          "passthrough",
-                          "sa&s",
-                          ipv,
-                          (int)rule->argsLen,
-                          rule->args) < 0)
-        goto cleanup;
-
-    if (error.level == VIR_ERR_ERROR) {
-        /*
-         * As of firewalld-0.3.9.3-1.fc20.noarch the name and
-         * message fields in the error look like
-         *
-         *    name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException"
-         * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete
-         *          INPUT --in-interface virbr0 --protocol udp --destination-port 53
-         *          --jump ACCEPT' failed: iptables: Bad rule (does a matching rule
-         *          exist in that chain?)."
-         *
-         * We'd like to only ignore DBus errors precisely related to the failure
-         * of iptables/ebtables commands. A well designed DBus interface would
-         * return specific named exceptions not the top level generic python dbus
-         * exception name. With this current scheme our only option is todo a
-         * sub-string match for 'COMMAND_FAILED' on the message. eg like
-         *
-         * if (ignoreErrors &&
-         *     STREQ(error.name,
-         *           "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") &&
-         *     STRPREFIX(error.message, "COMMAND_FAILED"))
-         *    ...
-         *
-         * But this risks our error detecting code being broken if firewalld changes
-         * ever alter the message string, so we're avoiding doing that.
-         */
-        if (ignoreErrors) {
-            VIR_DEBUG("Ignoring error '%s': '%s'",
-                      error.str1, error.message);
-        } else {
-            virReportErrorObject(&error);
-            goto cleanup;
-        }
-    } else {
-        if (virDBusMessageRead(reply, "s", output) < 0)
-            goto cleanup;
-    }
-
-    ret = 0;
-
- cleanup:
-    virResetError(&error);
-    virDBusMessageUnref(reply);
-    return ret;
+    /* wrapper necessary because virFirewallRule is a private struct */
+    return virFirewallDApplyRule(rule->layer, rule->args, rule->argsLen, ignoreErrors, output);
 }
 
 static int
diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
new file mode 100644
index 0000000000..0dc2b3de08
--- /dev/null
+++ b/src/util/virfirewalld.c
@@ -0,0 +1,128 @@
+/*
+ * virfirewalld.c: support for firewalld (https://firewalld.org)
+ *
+ * Copyright (C) 2019 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
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include <stdarg.h>
+
+#include "virfirewall.h"
+#include "virfirewalld.h"
+#include "virerror.h"
+#include "virutil.h"
+#include "virlog.h"
+#include "virdbus.h"
+
+#define VIR_FROM_THIS VIR_FROM_FIREWALLD
+
+VIR_LOG_INIT("util.firewalld");
+
+VIR_ENUM_DECL(virFirewallLayerFirewallD)
+VIR_ENUM_IMPL(virFirewallLayerFirewallD, VIR_FIREWALL_LAYER_LAST,
+              "eb", "ipv4", "ipv6")
+
+int
+virFirewallDStatus(void)
+{
+    return virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE);
+}
+
+
+int
+virFirewallDApplyRule(virFirewallLayer layer,
+                      char **args, size_t argsLen,
+                      bool ignoreErrors,
+                      char **output)
+{
+    const char *ipv = virFirewallLayerFirewallDTypeToString(layer);
+    DBusConnection *sysbus = virDBusGetSystemBus();
+    DBusMessage *reply = NULL;
+    virError error;
+    int ret = -1;
+
+    if (!sysbus)
+        return -1;
+
+    memset(&error, 0, sizeof(error));
+
+    if (!ipv) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Unknown firewall layer %d"),
+                       layer);
+        goto cleanup;
+    }
+
+    if (virDBusCallMethod(sysbus,
+                          &reply,
+                          &error,
+                          VIR_FIREWALL_FIREWALLD_SERVICE,
+                          "/org/fedoraproject/FirewallD1",
+                          "org.fedoraproject.FirewallD1.direct",
+                          "passthrough",
+                          "sa&s",
+                          ipv,
+                          (int)argsLen,
+                          args) < 0)
+        goto cleanup;
+
+    if (error.level == VIR_ERR_ERROR) {
+        /*
+         * As of firewalld-0.3.9.3-1.fc20.noarch the name and
+         * message fields in the error look like
+         *
+         *    name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException"
+         * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete
+         *          INPUT --in-interface virbr0 --protocol udp --destination-port 53
+         *          --jump ACCEPT' failed: iptables: Bad rule (does a matching rule
+         *          exist in that chain?)."
+         *
+         * We'd like to only ignore DBus errors precisely related to the failure
+         * of iptables/ebtables commands. A well designed DBus interface would
+         * return specific named exceptions not the top level generic python dbus
+         * exception name. With this current scheme our only option is todo a
+         * sub-string match for 'COMMAND_FAILED' on the message. eg like
+         *
+         * if (ignoreErrors &&
+         *     STREQ(error.name,
+         *           "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") &&
+         *     STRPREFIX(error.message, "COMMAND_FAILED"))
+         *    ...
+         *
+         * But this risks our error detecting code being broken if firewalld changes
+         * ever alter the message string, so we're avoiding doing that.
+         */
+        if (ignoreErrors) {
+            VIR_DEBUG("Ignoring error '%s': '%s'",
+                      error.str1, error.message);
+        } else {
+            virReportErrorObject(&error);
+            goto cleanup;
+        }
+    } else {
+        if (virDBusMessageRead(reply, "s", output) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    virResetError(&error);
+    virDBusMessageUnref(reply);
+    return ret;
+}
diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h
new file mode 100644
index 0000000000..c1c929399a
--- /dev/null
+++ b/src/util/virfirewalld.h
@@ -0,0 +1,33 @@
+/*
+ * virfirewalld.h: support for firewalld (https://firewalld.org)
+ *
+ * Copyright (C) 2019 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
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef LIBVIRT_VIRFIREWALLD_H
+# define LIBVIRT_VIRFIREWALLD_H
+
+# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"
+
+int virFirewallDStatus(void);
+
+int virFirewallDApplyRule(virFirewallLayer layer,
+                          char **args, size_t argsLen,
+                          bool ignoreErrors,
+                          char **output);
+
+#endif /* LIBVIRT_VIRFIREWALLD_H */
diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h
index efa94a7da4..7c31d0680d 100644
--- a/src/util/virfirewallpriv.h
+++ b/src/util/virfirewallpriv.h
@@ -27,8 +27,6 @@
 
 # include "virfirewall.h"
 
-# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"
-
 typedef enum {
     VIR_FIREWALL_BACKEND_AUTOMATIC,
     VIR_FIREWALL_BACKEND_DIRECT,
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index 63b9ced820..573ab1f9cd 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -27,6 +27,7 @@
 # include "vircommandpriv.h"
 # define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW
 # include "virfirewallpriv.h"
+# include "virfirewalld.h"
 # include "virmock.h"
 # define LIBVIRT_VIRDBUSPRIV_H_ALLOW
 # include "virdbuspriv.h"
-- 
2.20.1

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

On Wed, Jan 09, 2019 at 09:57:34PM -0500, Laine Stump wrote:
> Since I'm going to be adding at least one more firewalld-specific
> function, this seems like a good time to separate the code that's
> unique to firewalld from the more-generic "firewall" file.
> 
> Signed-off-by: Laine Stump <laine@laine.org>
> ---
>  include/libvirt/virterror.h |   1 +
>  src/libvirt_private.syms    |   3 +
>  src/util/Makefile.inc.am    |   2 +
>  src/util/virerror.c         |   1 +
>  src/util/virfirewall.c      |  86 ++----------------------
>  src/util/virfirewalld.c     | 128 ++++++++++++++++++++++++++++++++++++
>  src/util/virfirewalld.h     |  33 ++++++++++
>  src/util/virfirewallpriv.h  |   2 -
>  tests/virfirewalltest.c     |   1 +
>  9 files changed, 173 insertions(+), 84 deletions(-)
>  create mode 100644 src/util/virfirewalld.c
>  create mode 100644 src/util/virfirewalld.h

> +    if (error.level == VIR_ERR_ERROR) {
> +        /*
> +         * As of firewalld-0.3.9.3-1.fc20.noarch the name and
> +         * message fields in the error look like
> +         *
> +         *    name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException"
> +         * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete
> +         *          INPUT --in-interface virbr0 --protocol udp --destination-port 53
> +         *          --jump ACCEPT' failed: iptables: Bad rule (does a matching rule
> +         *          exist in that chain?)."
> +         *
> +         * We'd like to only ignore DBus errors precisely related to the failure
> +         * of iptables/ebtables commands. A well designed DBus interface would
> +         * return specific named exceptions not the top level generic python dbus
> +         * exception name. With this current scheme our only option is todo a
> +         * sub-string match for 'COMMAND_FAILED' on the message. eg like
> +         *
> +         * if (ignoreErrors &&
> +         *     STREQ(error.name,
> +         *           "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") &&
> +         *     STRPREFIX(error.message, "COMMAND_FAILED"))
> +         *    ...
> +         *
> +         * But this risks our error detecting code being broken if firewalld changes
> +         * ever alter the message string, so we're avoiding doing that.
> +         */

We really ought to ask the firewalld guys to improve this since
we have good communication with them now :-)

> +        if (ignoreErrors) {
> +            VIR_DEBUG("Ignoring error '%s': '%s'",
> +                      error.str1, error.message);
> +        } else {
> +            virReportErrorObject(&error);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (virDBusMessageRead(reply, "s", output) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virResetError(&error);
> +    virDBusMessageUnref(reply);
> +    return ret;
> +}


> diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h
> new file mode 100644
> index 0000000000..c1c929399a
> --- /dev/null
> +++ b/src/util/virfirewalld.h
> @@ -0,0 +1,33 @@


> +#ifndef LIBVIRT_VIRFIREWALLD_H
> +# define LIBVIRT_VIRFIREWALLD_H
> +
> +# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"

This is only needed for tests so should go in a priv.h header
as it was before.

> +
> +int virFirewallDStatus(void);

Perhaps  virFirewallDIsRegistered() ?

Could you put API docs for this since it has an unusal tri-state
return value.

> +
> +int virFirewallDApplyRule(virFirewallLayer layer,
> +                          char **args, size_t argsLen,
> +                          bool ignoreErrors,
> +                          char **output);

Would be nice to have API docs for this too

> +
> +#endif /* LIBVIRT_VIRFIREWALLD_H */
> diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h
> index efa94a7da4..7c31d0680d 100644
> --- a/src/util/virfirewallpriv.h
> +++ b/src/util/virfirewallpriv.h
> @@ -27,8 +27,6 @@
>  
>  # include "virfirewall.h"
>  
> -# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"
> -
>  typedef enum {
>      VIR_FIREWALL_BACKEND_AUTOMATIC,
>      VIR_FIREWALL_BACKEND_DIRECT,
> diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
> index 63b9ced820..573ab1f9cd 100644
> --- a/tests/virfirewalltest.c
> +++ b/tests/virfirewalltest.c
> @@ -27,6 +27,7 @@
>  # include "vircommandpriv.h"
>  # define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW
>  # include "virfirewallpriv.h"
> +# include "virfirewalld.h"

Should be virfirewalldpriv.h


Assuming those small changes are made

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On 1/9/19 9:57 PM, Laine Stump wrote:
> Since I'm going to be adding at least one more firewalld-specific
> function, this seems like a good time to separate the code that's
> unique to firewalld from the more-generic "firewall" file.
> 
> Signed-off-by: Laine Stump <laine@laine.org>
> ---
>  include/libvirt/virterror.h |   1 +
>  src/libvirt_private.syms    |   3 +
>  src/util/Makefile.inc.am    |   2 +
>  src/util/virerror.c         |   1 +
>  src/util/virfirewall.c      |  86 ++----------------------
>  src/util/virfirewalld.c     | 128 ++++++++++++++++++++++++++++++++++++
>  src/util/virfirewalld.h     |  33 ++++++++++
>  src/util/virfirewallpriv.h  |   2 -
>  tests/virfirewalltest.c     |   1 +
>  9 files changed, 173 insertions(+), 84 deletions(-)
>  create mode 100644 src/util/virfirewalld.c
>  create mode 100644 src/util/virfirewalld.h
> 

[...]

I was also looking as a learning opportunity, but I see Dan is reviewing
as well... Still I'll point out what I have in any case.

oddly enough I had similar thoughts regarding that moved hunk with the
(error.level == VIR_ERR_ERROR) checking...

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c3d6306809..583868f422 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1918,6 +1918,9 @@ virFirewallSetLockOverride;
>  virFirewallStartRollback;
>  virFirewallStartTransaction;
>  
> +# util/virfirewalld.h
> +virFirewallDApplyRule;
> +virFirewallDStatus;
>  

Just a syntax/spacing NIT here about 2 blank lines seems to be the norm
between groupings. So need one before and one after.

>  # util/virfirmware.h
>  virFirmwareFreeList;

[...]


John

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