[libvirt PATCH 07/14] src/util/virfirewalld: convert to use GLib DBus
Ján Tomko
jtomko at redhat.com
Mon Sep 21 07:57:33 UTC 2020
On a Thursday in 2020, Pavel Hrdina wrote:
>Signed-off-by: Pavel Hrdina <phrdina at 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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200921/b7399fcd/attachment-0001.sig>
More information about the libvir-list
mailing list