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

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- 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);

+ *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