[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt PATCH 07/14] src/util/virfirewalld: convert to use GLib DBus



On a Thursday in 2020, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina redhat com>
---
src/util/virfirewalld.c         | 197 ++++++++++++++++----------------
tests/meson.build               |   4 +-
tests/networkxml2firewalltest.c |  39 ++++---
tests/virfirewalltest.c         | 154 ++++++++++---------------
4 files changed, 180 insertions(+), 214 deletions(-)

diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
index c14a6b6e65..69c8b73da0 100644
--- a/src/util/virfirewalld.c
+++ b/src/util/virfirewalld.c
@@ -196,9 +195,9 @@ virFirewallDGetBackend(void)
int
virFirewallDGetZones(char ***zones, size_t *nzones)
{
-    DBusConnection *sysbus = virDBusGetSystemBus();
-    DBusMessage *reply = NULL;
-    int ret = -1;
+    GDBusConnection *sysbus = virGDBusGetSystemBus();
+    g_autoptr(GVariant) reply = NULL;
+    g_autoptr(GVariant) array = NULL;

    *nzones = 0;
    *zones = NULL;
@@ -206,23 +205,20 @@ virFirewallDGetZones(char ***zones, size_t *nzones)
    if (!sysbus)
        return -1;

-    if (virDBusCallMethod(sysbus,
-                          &reply,
-                          NULL,
-                          VIR_FIREWALL_FIREWALLD_SERVICE,
-                          "/org/fedoraproject/FirewallD1",
-                          "org.fedoraproject.FirewallD1.zone",
-                          "getZones",
-                          NULL) < 0)
-        goto cleanup;
+    if (virGDBusCallMethod(sysbus,
+                           &reply,
+                           NULL,
+                           VIR_FIREWALL_FIREWALLD_SERVICE,
+                           "/org/fedoraproject/FirewallD1",
+                           "org.fedoraproject.FirewallD1.zone",
+                           "getZones",
+                           NULL) < 0)
+        return -1;

-    if (virDBusMessageDecode(reply, "a&s", nzones, zones) < 0)
-        goto cleanup;
+    g_variant_get(reply, "(@as)", array);

Throughout the series you're not checking return values of
g_variant_get.

I'm getting assertion errors with firewalld disabled:
(process:156524): GLib-CRITICAL **: 09:56:49.398: g_variant_get_type: assertion 'value != NULL' failed

(process:156524): GLib-CRITICAL **: 09:56:49.399: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(process:156524): GLib-CRITICAL **: 09:56:49.399: g_variant_dup_strv: assertion 'g_variant_is_of_type (value, G_VARIANT_TYPE_STRING_ARRAY)' failed

Jano

+    *zones = g_variant_dup_strv(array, nzones);

-    ret = 0;
- cleanup:
-    virDBusMessageUnref(reply);
-    return ret;
+    return 0;
}


@@ -273,10 +269,10 @@ virFirewallDApplyRule(virFirewallLayer layer,
                      char **output)
{
    const char *ipv = virFirewallLayerFirewallDTypeToString(layer);
-    DBusConnection *sysbus = virDBusGetSystemBus();
-    DBusMessage *reply = NULL;
-    virError error;
-    int ret = -1;
+    GDBusConnection *sysbus = virGDBusGetSystemBus();
+    g_autoptr(GVariant) message = NULL;
+    g_autoptr(GVariant) reply = NULL;
+    g_autoptr(virError) error = NULL;

    if (!sysbus)
        return -1;
@@ -287,23 +283,27 @@ virFirewallDApplyRule(virFirewallLayer layer,
        virReportError(VIR_ERR_INTERNAL_ERROR,
                       _("Unknown firewall layer %d"),
                       layer);
-        goto cleanup;
+        return -1;
    }

-    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 (VIR_ALLOC(error) < 0)
+        return -1;

-    if (error.level == VIR_ERR_ERROR) {
+    message = g_variant_new("(s as)",
+                            ipv,
+                            g_variant_new_strv((const char * const*)args, argsLen));
+
+    if (virGDBusCallMethod(sysbus,
+                           &reply,
+                           error,
+                           VIR_FIREWALL_FIREWALLD_SERVICE,
+                           "/org/fedoraproject/FirewallD1",
+                           "org.fedoraproject.FirewallD1.direct",
+                           "passthrough",
+                           message) < 0)
+        return -1;
+
+    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
@@ -331,22 +331,16 @@ virFirewallDApplyRule(virFirewallLayer layer,
         */
        if (ignoreErrors) {
            VIR_DEBUG("Ignoring error '%s': '%s'",
-                      error.str1, error.message);
+                      error->str1, error->message);
        } else {
-            virReportErrorObject(&error);
-            goto cleanup;
+            virReportErrorObject(error);
+            return -1;
        }
    } else {
-        if (virDBusMessageDecode(reply, "s", output) < 0)
-            goto cleanup;
+        g_variant_get(reply, "(s)", output);
    }

-    ret = 0;
-
- cleanup:
-    virResetError(&error);
-    virDBusMessageUnref(reply);
-    return ret;
+    return 0;
}


@@ -354,19 +348,20 @@ int
virFirewallDInterfaceSetZone(const char *iface,
                             const char *zone)
{
-    DBusConnection *sysbus = virDBusGetSystemBus();
+    GDBusConnection *sysbus = virGDBusGetSystemBus();
+    g_autoptr(GVariant) message = NULL;

    if (!sysbus)
        return -1;

-    return virDBusCallMethod(sysbus,
+    message = g_variant_new("(ss)", zone, iface);
+
+    return virGDBusCallMethod(sysbus,
                             NULL,
                             NULL,
                             VIR_FIREWALL_FIREWALLD_SERVICE,
                             "/org/fedoraproject/FirewallD1",
                             "org.fedoraproject.FirewallD1.zone",
                             "changeZoneOfInterface",
-                             "ss",
-                             zone,
-                             iface);
+                             message);
}
diff --git a/tests/meson.build b/tests/meson.build
index 75bfb3effe..2c4f044d30 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -314,7 +314,7 @@ tests += [
  { 'name': 'virerrortest' },
  { 'name': 'virfilecachetest' },
  { 'name': 'virfiletest' },
-  { 'name': 'virfirewalltest', 'deps': [ dbus_dep ] },
+  { 'name': 'virfirewalltest' },
  { 'name': 'virhashtest' },
  { 'name': 'virhostcputest', 'link_whole': [ test_file_wrapper_lib ] },
  { 'name': 'virhostdevtest' },
@@ -400,7 +400,7 @@ endif
if conf.has('WITH_NETWORK')
  tests += [
    { 'name': 'networkxml2conftest', 'link_with': [ network_driver_impl ] },
-    { 'name': 'networkxml2firewalltest', 'link_with': [ network_driver_impl ], 'deps': [ dbus_dep ] },
+    { 'name': 'networkxml2firewalltest', 'link_with': [ network_driver_impl ] },
    { 'name': 'networkxml2xmltest', 'link_with': [ network_driver_impl ] },
  ]
endif
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 80e2d6a035..e0244f508e 100644
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -26,9 +26,7 @@

#if defined (__linux__)

-# if WITH_DBUS
-#  include <dbus/dbus.h>
-# endif
+# include <gio/gio.h>

# include "network/bridge_driver_platform.h"
# include "virbuffer.h"
@@ -48,21 +46,30 @@
#  error "test case not ported to this platform"
# endif

-# if WITH_DBUS
-VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
-                       DBusMessage *,
-                       DBusConnection *, connection,
-                       DBusMessage *, message,
-                       int, timeout_milliseconds,
-                       DBusError *, error)
+VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync,
+                       GVariant *,
+                       GDBusConnection *, connection,
+                       const gchar *, bus_name,
+                       const gchar *, object_path,
+                       const gchar *, interface_name,
+                       const gchar *, method_name,
+                       GVariant *, parameters,
+                       const GVariantType *, reply_type,
+                       GDBusCallFlags, flags,
+                       gint, timeout_msec,
+                       GCancellable *, cancellable,
+                       GError **, error)
{
-    VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block);
+    if (parameters)
+        g_variant_unref(parameters);

-    dbus_set_error_const(error, "org.freedesktop.error", "dbus is disabled");
+    VIR_MOCK_REAL_INIT(g_dbus_connection_call_sync);
+
+    *error = g_dbus_error_new_for_dbus_error("org.freedesktop.error",
+                                             "dbus is disabled");

    return NULL;
}
-# endif

static void
testCommandDryRun(const char *const*args G_GNUC_UNUSED,
@@ -197,11 +204,7 @@ mymain(void)
    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}

-# if WITH_DBUS
-VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virdbus"))
-# else
-VIR_TEST_MAIN(mymain)
-# endif
+VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virgdbus"))

#else /* ! defined (__linux__) */

diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index ce252bd0e0..607638e9d0 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -22,6 +22,8 @@

#if defined(__linux__)

+# include <gio/gio.h>
+
# include "virbuffer.h"
# define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW
# include "vircommandpriv.h"
@@ -30,15 +32,9 @@
# define LIBVIRT_VIRFIREWALLDPRIV_H_ALLOW
# include "virfirewalldpriv.h"
# include "virmock.h"
-# define LIBVIRT_VIRDBUSPRIV_H_ALLOW
-# include "virdbuspriv.h"

# define VIR_FROM_THIS VIR_FROM_FIREWALL

-# if WITH_DBUS
-#  include <dbus/dbus.h>
-# endif
-
static bool fwDisabled = true;
static virBufferPtr fwBuf;
static bool fwError;
@@ -66,64 +62,64 @@ static bool fwError;
    "Chain POSTROUTING (policy ACCEPT)\n" \
    "target     prot opt source               destination\n"

-# if WITH_DBUS
-VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
-                       DBusMessage *,
-                       DBusConnection *, connection,
-                       DBusMessage *, message,
-                       int, timeout_milliseconds,
-                       DBusError *, error)
+VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync,
+                       GVariant *,
+                       GDBusConnection *, connection,
+                       const gchar *, bus_name,
+                       const gchar *, object_path,
+                       const gchar *, interface_name,
+                       const gchar *, method_name,
+                       GVariant *, parameters,
+                       const GVariantType *, reply_type,
+                       GDBusCallFlags, flags,
+                       gint, timeout_msec,
+                       GCancellable *, cancellable,
+                       GError **, error)
{
-    DBusMessage *reply = NULL;
-    const char *service = dbus_message_get_destination(message);
-    const char *member = dbus_message_get_member(message);
-    size_t i;
-    size_t nargs = 0;
-    char **args = NULL;
-    char *type = NULL;
+    GVariant *reply = NULL;
+    g_autoptr(GVariant) params = parameters;

-    VIR_MOCK_REAL_INIT(dbus_connection_send_with_reply_and_block);
+    VIR_MOCK_REAL_INIT(g_dbus_connection_call_sync);

-    if (STREQ(service, "org.freedesktop.DBus") &&
-        STREQ(member, "ListNames")) {
-        const char *svc1 = "org.foo.bar.wizz";
-        const char *svc2 = VIR_FIREWALL_FIREWALLD_SERVICE;
-        DBusMessageIter iter;
-        DBusMessageIter sub;
-        reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN);
-        dbus_message_iter_init_append(reply, &iter);
-        dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-                                         "s", &sub);
+    if (STREQ(bus_name, "org.freedesktop.DBus") &&
+        STREQ(method_name, "ListNames")) {
+        GVariantBuilder builder;

-        if (!dbus_message_iter_append_basic(&sub,
-                                            DBUS_TYPE_STRING,
-                                            &svc1))
-            goto error;
-        if (!fwDisabled &&
-            !dbus_message_iter_append_basic(&sub,
-                                            DBUS_TYPE_STRING,
-                                            &svc2))
-            goto error;
-        dbus_message_iter_close_container(&iter, &sub);
-    } else if (STREQ(service, VIR_FIREWALL_FIREWALLD_SERVICE) &&
-               STREQ(member, "passthrough")) {
+        g_variant_builder_init(&builder, G_VARIANT_TYPE("(as)"));
+        g_variant_builder_open(&builder, G_VARIANT_TYPE("as"));
+
+        g_variant_builder_add(&builder, "s", "org.foo.bar.wizz");
+
+        if (!fwDisabled)
+            g_variant_builder_add(&builder, "s", VIR_FIREWALL_FIREWALLD_SERVICE);
+
+        g_variant_builder_close(&builder);
+
+        reply = g_variant_builder_end(&builder);
+    } else if (STREQ(bus_name, VIR_FIREWALL_FIREWALLD_SERVICE) &&
+               STREQ(method_name, "passthrough")) {
+        g_autoptr(GVariantIter) iter = NULL;
+        VIR_AUTOSTRINGLIST args = NULL;
+        size_t nargs = 0;
+        char *type = NULL;
+        char *item = NULL;
        bool isAdd = false;
        bool doError = false;
+        size_t i;

-        if (virDBusMessageDecode(message,
-                                 "sa&s",
-                                 &type,
-                                 &nargs,
-                                 &args) < 0)
-            goto error;
+        g_variant_get(params, "(&sas)", &type, &iter);

-        for (i = 0; i < nargs; i++) {
+        nargs = g_variant_iter_n_children(iter);
+
+        while (g_variant_iter_loop(iter, "s", &item)) {
            /* Fake failure on the command with this IP addr */
-            if (STREQ(args[i], "-A")) {
+            if (STREQ(item, "-A")) {
                isAdd = true;
-            } else if (isAdd && STREQ(args[i], "192.168.122.255")) {
+            } else if (isAdd && STREQ(item, "192.168.122.255")) {
                doError = true;
            }
+
+            virStringListAdd(&args, g_strdup(item));
        }

        if (fwBuf) {
@@ -134,61 +130,42 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
            else
                virBufferAddLit(fwBuf, EBTABLES_PATH);
        }
+
        for (i = 0; i < nargs; i++) {
            if (fwBuf) {
                virBufferAddLit(fwBuf, " ");
                virBufferEscapeShell(fwBuf, args[i]);
            }
        }
+
        if (fwBuf)
            virBufferAddLit(fwBuf, "\n");
+
        if (doError) {
-            dbus_set_error_const(error,
-                                 "org.firewalld.error",
-                                 "something bad happened");
+            if (error)
+                *error = g_dbus_error_new_for_dbus_error("org.firewalld.error",
+                                                         "something bad happened");
        } else {
            if (nargs == 1 &&
                STREQ(type, "ipv4") &&
                STREQ(args[0], "-L")) {
-                if (virDBusCreateReply(&reply,
-                                       "s", TEST_FILTER_TABLE_LIST) < 0)
-                    goto error;
+                reply = g_variant_new("(s)", TEST_FILTER_TABLE_LIST);
            } else if (nargs == 3 &&
                       STREQ(type, "ipv4") &&
                       STREQ(args[0], "-t") &&
                       STREQ(args[1], "nat") &&
                       STREQ(args[2], "-L")) {
-                if (virDBusCreateReply(&reply,
-                                       "s", TEST_NAT_TABLE_LIST) < 0)
-                    goto error;
+                reply = g_variant_new("(s)", TEST_NAT_TABLE_LIST);
            } else {
-                if (virDBusCreateReply(&reply,
-                                       "s", "success") < 0)
-                    goto error;
+                reply = g_variant_new("(s)", "success");
            }
        }
    } else {
-        reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN);
+        reply = g_variant_new("()");
    }

- cleanup:
-    VIR_FREE(type);
-    for (i = 0; i < nargs; i++)
-        VIR_FREE(args[i]);
-    VIR_FREE(args);
    return reply;
-
- error:
-    virDBusMessageUnref(reply);
-    reply = NULL;
-    if (error && !dbus_error_is_set(error))
-        dbus_set_error_const(error,
-                             "org.firewalld.error",
-                             "something unexpected happened");
-
-    goto cleanup;
}
-# endif

struct testFirewallData {
    virFirewallBackend tryBackend;
@@ -1073,8 +1050,7 @@ mymain(void)
            ret = -1; \
    } while (0)

-# if WITH_DBUS
-#  define RUN_TEST_FIREWALLD(name, method) \
+# define RUN_TEST_FIREWALLD(name, method) \
    do { \
        struct testFirewallData data; \
        data.tryBackend = VIR_FIREWALL_BACKEND_AUTOMATIC; \
@@ -1089,13 +1065,9 @@ mymain(void)
            ret = -1; \
    } while (0)

-#  define RUN_TEST(name, method) \
+# define RUN_TEST(name, method) \
    RUN_TEST_DIRECT(name, method); \
    RUN_TEST_FIREWALLD(name, method)
-# else /* ! WITH_DBUS */
-#  define RUN_TEST(name, method) \
-    RUN_TEST_DIRECT(name, method)
-# endif /* ! WITH_DBUS */

    virFirewallSetLockOverride(true);

@@ -1113,11 +1085,7 @@ mymain(void)
    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}

-# if WITH_DBUS
-VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virdbus"))
-# else
-VIR_TEST_MAIN(mymain)
-# endif
+VIR_TEST_MAIN_PRELOAD(mymain, VIR_TEST_MOCK("virgdbus"))

#else /* ! defined (__linux__) */

--
2.26.2

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]