From 2a6e7e917f9a567b7561dcfc9ff6de535861966f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Jan 2019 14:29:15 +0100 Subject: [PATCH 1/3] shared: add nm_g_variant_ref() and nm_g_variant_unref() helpers Akin to nm_g_object_ref() and nm_g_object_unref(). --- shared/nm-utils/nm-macros-internal.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 3578a55291..b993b3840c 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -1129,6 +1129,23 @@ nm_clear_g_cancellable (GCancellable **cancellable) /*****************************************************************************/ +static inline GVariant * +nm_g_variant_ref (GVariant *v) +{ + if (v) + g_variant_ref (v); + return v; +} + +static inline void +nm_g_variant_unref (GVariant *v) +{ + if (v) + g_variant_unref (v); +} + +/*****************************************************************************/ + /* Determine whether @x is a power of two (@x being an integer type). * Basically, this returns TRUE, if @x has exactly one bit set. * For negative values and zero, this always returns FALSE. */ From 3ae5c9d595c45b9253b9f1a2c3f56ebd2c9fb1d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 19 Dec 2018 17:35:17 +0100 Subject: [PATCH 2/3] core: cleanup error path in activation_add_done() Don't return success from a nested code path. Handle all errors first, and return early. Well, we cannot really return right away because we need to handle the failure. So, at least, check for errors and "goto fail". --- src/nm-manager.c | 49 ++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 2b14918683..945eb67233 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5278,32 +5278,37 @@ activation_add_done (NMSettings *settings, nm_utils_user_data_unpack (user_data, &self, &active, &persist_ptr); persist = GPOINTER_TO_INT (persist_ptr); - if (!error) { - nm_active_connection_set_settings_connection (active, new_connection); + if (error) + goto fail; - if (_internal_activate_generic (self, active, &local)) { - nm_settings_connection_update (new_connection, - NULL, - persist, - NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION | NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED, - "add-and-activate", - NULL); - g_dbus_method_invocation_return_value ( - context, - g_variant_new ("(oo)", - nm_dbus_object_get_path (NM_DBUS_OBJECT (new_connection)), - nm_dbus_object_get_path (NM_DBUS_OBJECT (active)))); - nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD_ACTIVATE, - nm_active_connection_get_settings_connection (active), - TRUE, - NULL, - nm_active_connection_get_subject (active), - NULL); - return; - } + nm_active_connection_set_settings_connection (active, new_connection); + + if (!_internal_activate_generic (self, active, &local)) { error = local; + goto fail; } + nm_settings_connection_update (new_connection, + NULL, + persist, + NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION | NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED, + "add-and-activate", + NULL); + + g_dbus_method_invocation_return_value (context, + g_variant_new ("(oo)", + nm_dbus_object_get_path (NM_DBUS_OBJECT (new_connection)), + nm_dbus_object_get_path (NM_DBUS_OBJECT (active)))); + + nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD_ACTIVATE, + nm_active_connection_get_settings_connection (active), + TRUE, + NULL, + nm_active_connection_get_subject (active), + NULL); + return; + +fail: nm_assert (error); nm_active_connection_set_state_fail (active, From fbb038af5e5d675c994de26da676edfd8e73ffbe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 20 Dec 2018 07:48:31 +0100 Subject: [PATCH 3/3] all: return output dictionary from "AddAndActivate2" Add a "a{sv}" output argument to "AddAndActivate2" D-Bus API. "AddAndActivate2" replaces "AddAndActivate" with more options. It also has a dictionary argument to be forward compatible so that we hopefully won't need an "AddAndActivate3". However, it lacked a similar output dictionary. Add it for future extensibility. I think this is really to workaround a shortcoming of D-Bus, which does provide strong typing and type information about its API, but does not allow to extend an existing API in a backward compatible manner. So we either resort to Method(), Method2(), Method3() variants, or a catch-all variant with a generic "a{sv}" input/output argument. In libnm, rename "nm_client_add_and_activate_connection_options()" to "nm_client_add_and_activate_connection2()". I think libnm API should have an obvious correspondence with D-Bus API. Or stated differently, if "AddAndActivateOptions" would be a better name, then the D-Bus API should be renamed. We should prefer one name over the other, but regardless of which is preferred, the naming for D-Bus and libnm API should correspond. In this case, I do think that AddAndActivate2() is a better name than AddAndActivateOptions(). Hence I rename the libnm API. Also, unless necessary, let libnm still call "AddAndActivate" instead of "AddAndActivate2". Our backward compatibility works the way that libnm requires a server version at least as new as itself. As such, libnm theoretically could assume that server version is new enough to support "AddAndActivate2" and could always use the more powerful variant. However, we don't need to break compatibility intentionally and for little gain. Here, it's easy to let libnm also handle old server API, by continuing to use "AddAndActivate" for nm_client_add_and_activate_connection(). Note that during package update, we don't restart the currently running NetworkManager instance. In such a scenario, it can easily happen that nmcli/libnm is newer than the server version. Let's try a bit harder to not break that. Changes as discussed in [1]. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/37#note_79876 --- .../org.freedesktop.NetworkManager.xml | 3 + libnm/libnm.ver | 4 +- libnm/nm-client.c | 83 ++++++---- libnm/nm-client.h | 23 +-- libnm/nm-manager.c | 144 ++++++++++++++---- libnm/nm-manager.h | 16 ++ src/nm-manager.c | 46 ++++-- tools/test-networkmanager-service.py | 9 +- 8 files changed, 241 insertions(+), 87 deletions(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index b8d0b5c125..efd866db47 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -119,6 +119,8 @@ @options: Further options for the method call. @path: Object path of the new connection that was just added. @active_connection: The path of the active connection object representing this active connection. + @result: a dictionary of additional output arguments for future extension. Currently not additional + output arguments are supported. Adds a new connection using the given details (if any) as a template (automatically filling in missing settings with the capabilities of the @@ -138,6 +140,7 @@ +