From 37e8c53eeed579fe34a68819cd12f3295d581394 Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Wed, 10 Oct 2018 17:10:01 +0200 Subject: [PATCH 01/16] core: Introduce helper class to track connection keep alive For P2P connections it makes sense to bind the connection to the status of the operation that is being done. One example is that a wifi display (miracast) P2P connection should be shut down when streaming fails for some reason. This new helper class allows binding a connection to the presence of a DBus path meaning that it will be torn down if the process disappears. --- Makefile.am | 2 + src/meson.build | 1 + src/nm-active-connection.c | 40 ++++++ src/nm-active-connection.h | 3 + src/nm-keep-alive.c | 251 +++++++++++++++++++++++++++++++++++++ src/nm-keep-alive.h | 56 +++++++++ src/nm-policy.c | 33 ++++- src/nm-types.h | 1 + 8 files changed, 385 insertions(+), 2 deletions(-) create mode 100644 src/nm-keep-alive.c create mode 100644 src/nm-keep-alive.h diff --git a/Makefile.am b/Makefile.am index 73e3c7041d..419006499e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1926,6 +1926,8 @@ src_libNetworkManager_la_SOURCES = \ src/nm-rfkill-manager.h \ src/nm-session-monitor.h \ src/nm-session-monitor.c \ + src/nm-keep-alive.c \ + src/nm-keep-alive.h \ src/nm-sleep-monitor.c \ src/nm-sleep-monitor.h \ src/nm-types.h \ diff --git a/src/meson.build b/src/meson.build index d58fb4d1f6..ca9919f65a 100644 --- a/src/meson.build +++ b/src/meson.build @@ -135,6 +135,7 @@ sources = files( 'nm-dispatcher.c', 'nm-firewall-manager.c', 'nm-hostname-manager.c', + 'nm-keep-alive.c', 'nm-manager.c', 'nm-netns.c', 'nm-pacrunner-manager.c', diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index a2f9ae4a2b..d8c886b147 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -30,6 +30,7 @@ #include "nm-auth-utils.h" #include "nm-auth-manager.h" #include "nm-auth-subject.h" +#include "nm-keep-alive.h" #include "NetworkManagerUtils.h" #include "nm-core-internal.h" @@ -75,6 +76,7 @@ typedef struct _NMActiveConnectionPrivate { gpointer user_data; } auth; + NMKeepAlive *keep_alive; } NMActiveConnectionPrivate; NM_GOBJECT_PROPERTIES_DEFINE (NMActiveConnection, @@ -103,6 +105,7 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMActiveConnection, PROP_INT_MASTER_READY, PROP_INT_ACTIVATION_TYPE, PROP_INT_ACTIVATION_REASON, + PROP_INT_KEEP_ALIVE, ); enum { @@ -173,6 +176,14 @@ NM_UTILS_FLAGS2STR_DEFINE_STATIC (_state_flags_to_string, NMActivationStateFlags /*****************************************************************************/ +static void +keep_alive_alive_changed (NMActiveConnection *ac, + GParamSpec *pspec, + NMKeepAlive *keep_alive) +{ + _notify (ac, PROP_INT_KEEP_ALIVE); +} + static void _settings_connection_updated (NMSettingsConnection *sett_conn, gboolean by_user, @@ -904,6 +915,14 @@ nm_active_connection_get_activation_reason (NMActiveConnection *self) return NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->activation_reason; } +gboolean +nm_active_connection_get_keep_alive (NMActiveConnection *self) +{ + NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + + return nm_keep_alive_is_alive (priv->keep_alive); +} + /*****************************************************************************/ static void @@ -1293,6 +1312,9 @@ get_property (GObject *object, guint prop_id, case PROP_INT_MASTER_READY: g_value_set_boolean (value, priv->master_ready); break; + case PROP_INT_KEEP_ALIVE: + g_value_set_boolean (value, nm_keep_alive_is_alive (priv->keep_alive)); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); break; @@ -1400,6 +1422,12 @@ nm_active_connection_init (NMActiveConnection *self) priv->activation_type = NM_ACTIVATION_TYPE_MANAGED; priv->version_id = _version_id_new (); + + priv->keep_alive = nm_keep_alive_new (TRUE); + g_signal_connect_object (priv->keep_alive, "notify::" NM_KEEP_ALIVE_ALIVE, + (GCallback) keep_alive_alive_changed, + self, + G_CONNECT_SWAPPED); } static void @@ -1431,6 +1459,12 @@ constructed (GObject *object) g_return_if_fail (priv->subject); g_return_if_fail (priv->activation_reason != NM_ACTIVATION_REASON_UNSET); + + if (priv->activation_reason == NM_ACTIVATION_REASON_AUTOCONNECT || + priv->activation_reason == NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES) { + nm_keep_alive_set_settings_connection_watch_visible (priv->keep_alive, priv->settings_connection.obj); + nm_keep_alive_sink (priv->keep_alive); + } } static void @@ -1474,6 +1508,7 @@ finalize (GObject *object) NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); nm_dbus_track_obj_path_set (&priv->settings_connection, NULL, FALSE); + g_clear_object (&priv->keep_alive); G_OBJECT_CLASS (nm_active_connection_parent_class)->finalize (object); } @@ -1684,6 +1719,11 @@ nm_active_connection_class_init (NMActiveConnectionClass *ac_class) G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); + obj_properties[PROP_INT_KEEP_ALIVE] = + g_param_spec_boolean (NM_ACTIVE_CONNECTION_INT_KEEP_ALIVE, "", "", + TRUE, G_PARAM_READABLE | + G_PARAM_STATIC_STRINGS); + g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties); signals[DEVICE_CHANGED] = diff --git a/src/nm-active-connection.h b/src/nm-active-connection.h index b28c2b02fd..85689a6bfd 100644 --- a/src/nm-active-connection.h +++ b/src/nm-active-connection.h @@ -59,6 +59,7 @@ #define NM_ACTIVE_CONNECTION_INT_MASTER_READY "int-master-ready" #define NM_ACTIVE_CONNECTION_INT_ACTIVATION_TYPE "int-activation-type" #define NM_ACTIVE_CONNECTION_INT_ACTIVATION_REASON "int-activation-reason" +#define NM_ACTIVE_CONNECTION_INT_KEEP_ALIVE "int-keep-alive" /* Signals */ #define NM_ACTIVE_CONNECTION_STATE_CHANGED "state-changed" @@ -185,6 +186,8 @@ NMActivationType nm_active_connection_get_activation_type (NMActiveConnection *s NMActivationReason nm_active_connection_get_activation_reason (NMActiveConnection *self); +gboolean nm_active_connection_get_keep_alive (NMActiveConnection *self); + void nm_active_connection_clear_secrets (NMActiveConnection *self); #endif /* __NETWORKMANAGER_ACTIVE_CONNECTION_H__ */ diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c new file mode 100644 index 0000000000..be6dc2304b --- /dev/null +++ b/src/nm-keep-alive.c @@ -0,0 +1,251 @@ +/* + * NetworkManager -- Inhibition management + * + * 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 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, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * Copyright 2018 Red Hat, Inc. + */ + +#include "nm-default.h" + +#include "nm-keep-alive.h" +#include "settings/nm-settings-connection.h" + +#include + +/*****************************************************************************/ + +NM_GOBJECT_PROPERTIES_DEFINE (NMKeepAlive, + PROP_ALIVE, +); + +typedef struct { + gboolean floating; + gboolean forced; + NMSettingsConnection *connection; + GDBusConnection *dbus_connection; + char *dbus_client; + + guint subscription_id; +} NMKeepAlivePrivate; + +struct _NMKeepAlive { + GObject parent; + NMKeepAlivePrivate _priv; +}; + +struct _NMKeepAliveClass { + GObjectClass parent; +}; + +G_DEFINE_TYPE (NMKeepAlive, nm_keep_alive, G_TYPE_OBJECT) + +#define NM_KEEP_ALIVE_GET_PRIVATE(self) _NM_GET_PRIVATE (self, NMKeepAlive, NM_IS_KEEP_ALIVE) + +/*****************************************************************************/ + +#define _NMLOG_DOMAIN LOGD_CORE +#define _NMLOG(level, ...) __NMLOG_DEFAULT (level, _NMLOG_DOMAIN, "keep-alive", __VA_ARGS__) + +/*****************************************************************************/ + +NMKeepAlive* nm_keep_alive_new (gboolean floating) +{ + NMKeepAlive *res = g_object_new (NM_TYPE_KEEP_ALIVE, NULL); + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (res); + + priv->floating = floating; + + return res; +} + +gboolean nm_keep_alive_is_alive (NMKeepAlive *self) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + if (priv->floating || priv->forced) + return TRUE; + + if (priv->connection && NM_FLAGS_HAS (nm_settings_connection_get_flags (priv->connection), + NM_SETTINGS_CONNECTION_INT_FLAGS_VISIBLE)) + return TRUE; + + if (priv->dbus_client) + return TRUE; + + return FALSE; +} + +void nm_keep_alive_sink (NMKeepAlive *self) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + priv->floating = FALSE; + + _notify (self, PROP_ALIVE); +} + +void nm_keep_alive_set_forced (NMKeepAlive *self, gboolean forced) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + priv->forced = forced; + + _notify (self, PROP_ALIVE); +} + +static void +connection_flags_changed (NMSettingsConnection *connection, + NMKeepAlive *self) +{ + _notify (self, PROP_ALIVE); +} + + +void +nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, + NMSettingsConnection *connection) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + if (priv->connection) { + g_signal_handlers_disconnect_by_data (priv->connection, self); + priv->connection = NULL; + } + + priv->connection = g_object_ref (connection); + g_signal_connect_object (priv->connection, NM_SETTINGS_CONNECTION_FLAGS_CHANGED, + G_CALLBACK (connection_flags_changed), self, 0); + + _notify (self, PROP_ALIVE); +} + +static void +cleanup_dbus_watch (NMKeepAlive *self) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + _LOGD ("Cleanup DBus client watch"); + + g_clear_pointer (&priv->dbus_client, g_free); + + if (priv->dbus_connection) + g_dbus_connection_signal_unsubscribe (priv->dbus_connection, + priv->subscription_id); + g_clear_object (&priv->dbus_connection); +} + +static void +name_owner_changed_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, + gpointer user_data) +{ + NMKeepAlive *self = NM_KEEP_ALIVE (user_data); + + const char *old_owner; + const char *new_owner; + + g_variant_get (parameters, "(&s&s&s)", NULL, &old_owner, &new_owner); + + if (g_strcmp0 (new_owner, "") != 0) + return; + + _LOGD ("DBus client for keep alive disappeared from bus"); + + cleanup_dbus_watch (self); + + _notify (self, PROP_ALIVE); +} + +void +nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, + GDBusConnection *connection, + const char *client_address, + GError **error) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + cleanup_dbus_watch (self); + + if (client_address == NULL) + return; + + _LOGD ("Registering dbus client watch for keep alive"); + + priv->dbus_client = g_strdup (client_address); + priv->dbus_connection = g_object_ref (connection); + priv->subscription_id = + g_dbus_connection_signal_subscribe (connection, "org.freedesktop.DBus", + "org.freedesktop.DBus", "NameOwnerChanged", "/org/freedesktop/DBus", + priv->dbus_client, G_DBUS_SIGNAL_FLAGS_NONE, + name_owner_changed_cb, self, NULL); +} + +static void +get_property (GObject *object, + guint prop_id, + GValue *value, + GParamSpec *pspec) +{ + NMKeepAlive *self = NM_KEEP_ALIVE (object); + + switch (prop_id) { + case PROP_ALIVE: + g_value_set_boolean (value, nm_keep_alive_is_alive (self)); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } +} + +static void +nm_keep_alive_init (NMKeepAlive *self) +{ + /* Nothing to do */ +} + +static void +dispose (GObject *object) +{ + NMKeepAlive *self = NM_KEEP_ALIVE (object); + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + g_clear_object (&priv->connection); + + cleanup_dbus_watch (self); +} + +static void +nm_keep_alive_class_init (NMKeepAliveClass *keep_alive_class) +{ + GObjectClass *object_class = G_OBJECT_CLASS (keep_alive_class); + + object_class->get_property = get_property; + object_class->dispose = dispose; + + obj_properties[PROP_ALIVE] = + g_param_spec_string (NM_KEEP_ALIVE_ALIVE, "", "", + NULL, + G_PARAM_READABLE | + G_PARAM_STATIC_STRINGS); + + g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties); +} diff --git a/src/nm-keep-alive.h b/src/nm-keep-alive.h new file mode 100644 index 0000000000..5ca02a3c5a --- /dev/null +++ b/src/nm-keep-alive.h @@ -0,0 +1,56 @@ +/* + * NetworkManager -- Inhibition management + * + * 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 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, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * Copyright 2018 Red Hat, Inc. + */ + +#ifndef __NETWORKMANAGER_KEEP_ALIVE_H__ +#define __NETWORKMANAGER_KEEP_ALIVE_H__ + +#define NM_TYPE_KEEP_ALIVE (nm_keep_alive_get_type ()) +#define NM_KEEP_ALIVE(o) (G_TYPE_CHECK_INSTANCE_CAST ((o), NM_TYPE_KEEP_ALIVE, NMKeepAlive)) +#define NM_KEEP_ALIVE_CLASS(k) (G_TYPE_CHECK_CLASS_CAST ((k), NM_TYPE_KEEP_ALIVE, NMKeepAliveClass)) +#define NM_KEEP_ALIVE_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), NM_TYPE_KEEP_ALIVE, NMKeepAliveClass)) +#define NM_IS_KEEP_ALIVE(o) (G_TYPE_CHECK_INSTANCE_TYPE ((o), NM_TYPE_KEEP_ALIVE)) +#define NM_IS_KEEP_ALIVE_CLASS(k) (G_TYPE_CHECK_CLASS_TYPE ((k), NM_TYPE_KEEP_ALIVE)) + + +#define NM_KEEP_ALIVE_ALIVE "alive" + +typedef struct _NMKeepAliveClass NMKeepAliveClass; + +GType nm_keep_alive_get_type (void) G_GNUC_CONST; + +NMKeepAlive* nm_keep_alive_new (gboolean floating); + +gboolean nm_keep_alive_is_alive (NMKeepAlive *self); + +void nm_keep_alive_sink (NMKeepAlive *self); + +void nm_keep_alive_set_forced (NMKeepAlive *self, + gboolean forced); + +void nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, + NMSettingsConnection *connection); + +void nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, + GDBusConnection *connection, + const char *client_address, + GError **error); + +#endif /* __NETWORKMANAGER_KEEP_ALIVE_H__ */ diff --git a/src/nm-policy.c b/src/nm-policy.c index 5ac40dca53..323df112de 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -2182,6 +2182,32 @@ active_connection_state_changed (NMActiveConnection *active, process_secondaries (self, active, FALSE); } +static void +active_connection_keep_alive_changed (NMActiveConnection *ac, + GParamSpec *pspec, + NMPolicy *self) +{ + NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); + GError *error = NULL; + + /* The connection does not have a reason to stay alive anymore. */ + if (!nm_active_connection_get_keep_alive (ac)) { + if (nm_active_connection_get_state (ac) <= NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { + if (!nm_manager_deactivate_connection (priv->manager, + ac, + NM_DEVICE_STATE_REASON_CONNECTION_REMOVED, + &error)) { + _LOGW (LOGD_DEVICE, "connection '%s' is no longer kept alive, but error deactivating it: (%d) %s", + nm_active_connection_get_settings_connection_id (ac), + error ? error->code : -1, + error ? error->message : "(unknown)"); + g_clear_error (&error); + } + } + } + +} + static void active_connection_added (NMManager *manager, NMActiveConnection *active, @@ -2202,6 +2228,10 @@ active_connection_added (NMManager *manager, g_signal_connect (active, "notify::" NM_ACTIVE_CONNECTION_STATE, G_CALLBACK (active_connection_state_changed), self); + g_signal_connect (active, "notify::" NM_ACTIVE_CONNECTION_INT_KEEP_ALIVE, + G_CALLBACK (active_connection_keep_alive_changed), + self); + active_connection_keep_alive_changed (active, NULL, self); } static void @@ -2404,8 +2434,7 @@ connection_flags_changed (NMSettings *settings, NM_SETTINGS_CONNECTION_INT_FLAGS_VISIBLE)) { if (!nm_settings_connection_autoconnect_is_blocked (connection)) schedule_activate_all (self); - } else - _deactivate_if_active (self, connection); + } } static void diff --git a/src/nm-types.h b/src/nm-types.h index 676ca64169..277e0f6ccf 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -51,6 +51,7 @@ typedef struct _NMPolicy NMPolicy; typedef struct _NMRfkillManager NMRfkillManager; typedef struct _NMPacrunnerManager NMPacrunnerManager; typedef struct _NMSessionMonitor NMSessionMonitor; +typedef struct _NMKeepAlive NMKeepAlive; typedef struct _NMSleepMonitor NMSleepMonitor; typedef struct _NMLldpListener NMLldpListener; typedef struct _NMConfigDeviceStateData NMConfigDeviceStateData; From 43c19d755a59e07fb6135c6885b1d3723ec15498 Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Tue, 30 Oct 2018 16:40:40 +0100 Subject: [PATCH 02/16] core: Add an AddAndActivateConnection2 routine with options parameter This adds a new routine to be able to handle an arbitrary set of further options for AddAndActivateConnection. Note that no options are accepted for now. --- .../org.freedesktop.NetworkManager.xml | 28 +++++++++++++ src/nm-manager.c | 41 ++++++++++++++++++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index 6c6ec282a6..356297722f 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -100,6 +100,8 @@ (automatically filling in missing settings with the capabilities of the given device and specific object), then activate the new connection. Cannot be used for VPN connections at this time. + + See also AddAndActivateConnection2. --> @@ -109,6 +111,32 @@ + + + + + + + + + + diff --git a/src/nm-manager.c b/src/nm-manager.c index b8e3e3671f..a9b12878ac 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -95,6 +95,7 @@ typedef struct { struct { GDBusMethodInvocation *invocation; NMConnection *connection; + NMSettingsConnectionPersistMode persist; } add_and_activate; }; } ac_auth; @@ -370,6 +371,7 @@ static void _add_and_activate_auth_done (NMManager *self, NMActiveConnection *active, NMConnection *connection, GDBusMethodInvocation *invocation, + NMSettingsConnectionPersistMode persist, gboolean success, const char *error_desc); static void _activation_auth_done (NMManager *self, @@ -484,7 +486,8 @@ static AsyncOpData * _async_op_data_new_ac_auth_add_and_activate (NMManager *self, NMActiveConnection *active_take, GDBusMethodInvocation *invocation_take, - NMConnection *connection_take) + NMConnection *connection_take, + NMSettingsConnectionPersistMode persist) { AsyncOpData *async_op_data; @@ -494,6 +497,7 @@ _async_op_data_new_ac_auth_add_and_activate (NMManager *self, async_op_data->ac_auth.active = active_take; async_op_data->ac_auth.add_and_activate.invocation = invocation_take; async_op_data->ac_auth.add_and_activate.connection = connection_take; + async_op_data->ac_auth.add_and_activate.persist = persist; c_list_link_tail (&NM_MANAGER_GET_PRIVATE (self)->async_op_lst_head, &async_op_data->async_op_lst); return async_op_data; } @@ -533,6 +537,7 @@ _async_op_complete_ac_auth_cb (NMActiveConnection *active, async_op_data->ac_auth.active, async_op_data->ac_auth.add_and_activate.connection, async_op_data->ac_auth.add_and_activate.invocation, + async_op_data->ac_auth.add_and_activate.persist, success, error_desc); g_object_unref (async_op_data->ac_auth.add_and_activate.connection); @@ -5117,8 +5122,11 @@ activation_add_done (NMSettings *settings, NMManager *self; gs_unref_object NMActiveConnection *active = NULL; gs_free_error GError *local = NULL; + gpointer persist_ptr; + NMSettingsConnectionPersistMode persist; - nm_utils_user_data_unpack (user_data, &self, &active); + 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); @@ -5126,7 +5134,7 @@ activation_add_done (NMSettings *settings, if (_internal_activate_generic (self, active, &local)) { nm_settings_connection_update (new_connection, NULL, - NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK, + persist, NM_SETTINGS_CONNECTION_COMMIT_REASON_USER_ACTION | NM_SETTINGS_CONNECTION_COMMIT_REASON_ID_CHANGED, "add-and-activate", NULL); @@ -5167,6 +5175,7 @@ _add_and_activate_auth_done (NMManager *self, NMActiveConnection *active, NMConnection *connection, GDBusMethodInvocation *invocation, + NMSettingsConnectionPersistMode persist, gboolean success, const char *error_desc) { @@ -5199,7 +5208,8 @@ _add_and_activate_auth_done (NMManager *self, invocation, activation_add_done, nm_utils_user_data_pack (self, - g_object_ref (active))); + g_object_ref (active), + GINT_TO_POINTER (persist))); } static void @@ -5224,6 +5234,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, const char *device_path; const char *specific_object_path; gs_free NMConnection **conns = NULL; + NMSettingsConnectionPersistMode persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; if (g_strcmp0 (method_info->parent.name, "AddAndActivateConnection2") == 0) g_variant_get (parameters, "(@a{sa{sv}}&o&o@a{sv})", &settings, &device_path, &specific_object_path, &options); @@ -5235,17 +5246,37 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, const char *option_name; GVariant *option_value; - /* Just a commit to prepare the support, error out if any option is passed. */ g_variant_iter_init (&iter, options); while (g_variant_iter_next (&iter, "{&sv}", &option_name, &option_value)) { gs_unref_variant GVariant *option_value_free = NULL; + const char *s; option_value_free = option_value; - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "Unknown extra option passed."); - goto error; + if ((g_strcmp0 (option_name, "persist") == 0) && + g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { + s = g_variant_get_string (option_value, NULL); + + if (g_strcmp0 (s, "volatile") == 0) { + persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_VOLATILE_ONLY; + } else if (g_strcmp0 (s, "memory") == 0) { + persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; + } else if (g_strcmp0 (s, "disk") == 0) { + persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + } else { + error = g_error_new_literal (NM_MANAGER_ERROR, + NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "Option \"persist\" must be one of \"volatile\", \"memory\" or \"disk\"."); + goto error; + } + + } else { + /* Unknown argument */ + error = g_error_new_literal (NM_MANAGER_ERROR, + NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "Unknown extra option passed."); + goto error; + } } } @@ -5325,7 +5356,8 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, _async_op_data_new_ac_auth_add_and_activate (self, active, invocation, - incompl_conn)); + incompl_conn, + persist)); /* we passed the pointers on to _async_op_data_new_ac_auth_add_and_activate() */ g_steal_pointer (&incompl_conn); From eb883e34a5889812d35e5c2d7bc6051723001a3a Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Fri, 26 Oct 2018 13:17:31 +0200 Subject: [PATCH 04/16] core: Add option to AddAndActivateConnection2 to bind the lifetime This allows binding the lifetime of the created connection to the existance of the requesting dbus client. This feature is useful if one has a service specific connection (e.g. P2P wireless) which will not be useful without the specific service. This is simply a mechanism to ensure proper connection cleanup if the requesting service has a failure. --- .../org.freedesktop.NetworkManager.xml | 1 + src/nm-active-connection.c | 17 ++++++++++++++ src/nm-active-connection.h | 4 ++++ src/nm-manager.c | 23 ++++++++++++++++++- 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index 86131c8d33..715e182eda 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -129,6 +129,7 @@ parameters. At this time the following options are supported: * persist: A string value of either "disk" (default), "memory" or "volatile". If "memory" is passed, the connection will not be saved to disk. If "volatile" is passed, the connection will not be saved to disk and will be destroyed when disconnected. + * bind: Bind the connections lifetime. Set to "dbus-name" to automatically disconnect when the requesting process disappears from the bus. The default of "none" means the connection is kept alive normally. If restricted, then persist must be set to "volatile". --> diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index d8c886b147..84c86ce5de 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1014,6 +1014,23 @@ nm_active_connection_set_parent (NMActiveConnection *self, NMActiveConnection *p g_object_weak_ref ((GObject *) priv->parent, parent_destroyed, self); } +/** + * nm_active_connection_bind_dbus_client: + * @self: the #NMActiveConnection + * @dbus_client: The dbus client to watch. + * + * Binds the lifetime of this active connection to the given dbus client. If + * the dbus client disappears, then the connection will be disconnected. + */ +void +nm_active_connection_bind_dbus_client (NMActiveConnection *self, GDBusConnection *dbus_con, const char *dbus_client) +{ + NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + + nm_keep_alive_set_dbus_client_watch (priv->keep_alive, dbus_con, dbus_client, NULL); + nm_keep_alive_sink (priv->keep_alive); +} + /*****************************************************************************/ static void diff --git a/src/nm-active-connection.h b/src/nm-active-connection.h index 85689a6bfd..5b2e421507 100644 --- a/src/nm-active-connection.h +++ b/src/nm-active-connection.h @@ -190,4 +190,8 @@ gboolean nm_active_connection_get_keep_alive (NMActiveConnection *self); void nm_active_connection_clear_secrets (NMActiveConnection *self); +void nm_active_connection_bind_dbus_client (NMActiveConnection *self, + GDBusConnection *dbus_con, + const char *dbus_name); + #endif /* __NETWORKMANAGER_ACTIVE_CONNECTION_H__ */ diff --git a/src/nm-manager.c b/src/nm-manager.c index a9b12878ac..e5a0964192 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5224,7 +5224,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, NMManager *self = NM_MANAGER (obj); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); gs_unref_object NMConnection *incompl_conn = NULL; - NMActiveConnection *active = NULL; + gs_unref_object NMActiveConnection *active = NULL; gs_unref_object NMAuthSubject *subject = NULL; GError *error = NULL; NMDevice *device = NULL; @@ -5235,6 +5235,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, const char *specific_object_path; gs_free NMConnection **conns = NULL; NMSettingsConnectionPersistMode persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; + const char *bind_lifetime = "none"; if (g_strcmp0 (method_info->parent.name, "AddAndActivateConnection2") == 0) g_variant_get (parameters, "(@a{sa{sv}}&o&o@a{sv})", &settings, &device_path, &specific_object_path, &options); @@ -5270,6 +5271,19 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, goto error; } + } else if ((g_strcmp0 (option_name, "bind") == 0) && + g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { + s = g_variant_get_string (option_value, NULL); + + if (!NM_IN_STRSET (s, "dbus-client", "none")) { + error = g_error_new_literal (NM_MANAGER_ERROR, + NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "Option \"bind\" must be one of \"dbus-client\" or \"none\"."); + goto error; + } + + bind_lifetime = s; + } else { /* Unknown argument */ error = g_error_new_literal (NM_MANAGER_ERROR, @@ -5350,6 +5364,13 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!active) goto error; + if (g_strcmp0 (bind_lifetime, "dbus-client") == 0) { + if (persist != NM_SETTINGS_CONNECTION_PERSIST_MODE_VOLATILE_ONLY) + goto error; + + nm_active_connection_bind_dbus_client (active, dbus_connection, sender); + } + nm_active_connection_authorize (active, incompl_conn, _async_op_complete_ac_auth_cb, From 00236ef97726f6bc30f439fdc41314cdf18997aa Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Fri, 26 Oct 2018 13:44:38 +0200 Subject: [PATCH 05/16] libnm: Add support to pass options to AddAndActivateConnection This adds the new methods nm_client_add_and_activate_connection_options_* and ports the existing methods to use the new AddAndActivateConnection2 call rather than AddAndActivateConnection, allowing further parameters to be passed in. --- libnm/libnm.ver | 2 + libnm/nm-client.c | 110 ++++++++++++++++++++++++++- libnm/nm-client.h | 12 +++ libnm/nm-manager.c | 16 ++-- libnm/nm-manager.h | 1 + tools/test-networkmanager-service.py | 6 ++ 6 files changed, 140 insertions(+), 7 deletions(-) diff --git a/libnm/libnm.ver b/libnm/libnm.ver index cf57b12405..269f406cd3 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1447,5 +1447,7 @@ global: libnm_1_16_0 { global: + nm_client_add_and_activate_connection_options_async; + nm_client_add_and_activate_connection_options_finish; nm_device_get_connectivity; } libnm_1_14_0; diff --git a/libnm/nm-client.c b/libnm/nm-client.c index a669f3c5a9..576fbacbc4 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -1233,7 +1233,7 @@ nm_client_add_and_activate_connection_async (NMClient *client, if (cancellable) g_simple_async_result_set_check_cancellable (simple, cancellable); nm_manager_add_and_activate_connection_async (NM_CLIENT_GET_PRIVATE (client)->manager, - partial, device, specific_object, + partial, device, specific_object, NULL, cancellable, add_activate_cb, simple); } @@ -1268,6 +1268,114 @@ nm_client_add_and_activate_connection_finish (NMClient *client, return g_object_ref (g_simple_async_result_get_op_res_gpointer (simple)); } +/** + * nm_client_add_and_activate_connection_options_async: + * @client: a #NMClient + * @partial: (allow-none): an #NMConnection to add; the connection may be + * partially filled (or even %NULL) and will be completed by NetworkManager + * using the given @device and @specific_object before being added + * @device: the #NMDevice + * @specific_object: (allow-none): the object path of a connection-type-specific + * object this activation should use. This parameter is currently ignored for + * wired and mobile broadband connections, and the value of %NULL should be used + * (ie, no specific object). For Wi-Fi or WiMAX connections, pass the object + * path of a #NMAccessPoint or #NMWimaxNsp owned by @device, which you can + * get using nm_object_get_path(), and which will be used to complete the + * details of the newly added connection. + * @options: a #GVariant containing a dictionary with options, or %NULL + * @cancellable: a #GCancellable, or %NULL + * @callback: callback to be called when the activation has started + * @user_data: caller-specific data passed to @callback + * + * Adds a new connection using the given details (if any) as a template, + * automatically filling in missing settings with the capabilities of the given + * device and specific object. The new connection is then asynchronously + * activated as with nm_client_activate_connection_async(). Cannot be used for + * VPN connections at this time. + * + * Note that the callback is invoked when NetworkManager has started activating + * the new connection, not when it finishes. You can used the returned + * #NMActiveConnection object (in particular, #NMActiveConnection:state) to + * track the activation to its completion. + * + * This is identitcal to nm_client_add_and_activate_connection_async() but takes + * a further @options parameter. Currently the following options are supported + * by the daemon: + * * "persist": A string describing how the connection should be stored. + * The default is "disk", but it can be modified to "memory" (until + * the daemon quits) or "volatile" (will be deleted on disconnect). + * * "bind": Bind the connection lifetime to something. The default is "none", + * meaning an explicit disconnect is needed. The value "dbus-client" + * means the connection will automatically be closed when the calling + * DBus client disappears from the system bus. + * A non-default "bind" option must always be used together with + * "persist" set to "volatile". + * + * Since: 1.16 + **/ +void +nm_client_add_and_activate_connection_options_async (NMClient *client, + NMConnection *partial, + NMDevice *device, + const char *specific_object, + GVariant *options, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + GSimpleAsyncResult *simple; + GError *error = NULL; + + g_return_if_fail (NM_IS_CLIENT (client)); + g_return_if_fail (NM_IS_DEVICE (device)); + if (partial) + g_return_if_fail (NM_IS_CONNECTION (partial)); + + if (!_nm_client_check_nm_running (client, &error)) { + g_simple_async_report_take_gerror_in_idle (G_OBJECT (client), callback, user_data, error); + return; + } + + simple = g_simple_async_result_new (G_OBJECT (client), callback, user_data, + nm_client_add_and_activate_connection_options_async); + if (cancellable) + g_simple_async_result_set_check_cancellable (simple, cancellable); + nm_manager_add_and_activate_connection_async (NM_CLIENT_GET_PRIVATE (client)->manager, + partial, device, specific_object, options, + cancellable, add_activate_cb, simple); +} + +/** + * nm_client_add_and_activate_connection_options_finish: + * @client: an #NMClient + * @result: the result passed to the #GAsyncReadyCallback + * @error: location for a #GError, or %NULL + * + * Gets the result of a call to nm_client_add_and_activate_connection_options_async(). + * + * You can call nm_active_connection_get_connection() on the returned + * #NMActiveConnection to find the path of the created #NMConnection. + * + * Returns: (transfer full): the new #NMActiveConnection on success, %NULL on + * failure, in which case @error will be set. + **/ +NMActiveConnection * +nm_client_add_and_activate_connection_options_finish (NMClient *client, + GAsyncResult *result, + GError **error) +{ + GSimpleAsyncResult *simple; + + g_return_val_if_fail (NM_IS_CLIENT (client), NULL); + g_return_val_if_fail (G_IS_SIMPLE_ASYNC_RESULT (result), NULL); + + simple = G_SIMPLE_ASYNC_RESULT (result); + if (g_simple_async_result_propagate_error (simple, error)) + return NULL; + else + return g_object_ref (g_simple_async_result_get_op_res_gpointer (simple)); +} + /** * nm_client_deactivate_connection: * @client: a #NMClient diff --git a/libnm/nm-client.h b/libnm/nm-client.h index 6259b1227b..dc5649f3cf 100644 --- a/libnm/nm-client.h +++ b/libnm/nm-client.h @@ -339,6 +339,18 @@ NMActiveConnection *nm_client_add_and_activate_connection_finish (NMClient *clie GAsyncResult *result, GError **error); +void nm_client_add_and_activate_connection_options_async (NMClient *client, + NMConnection *partial, + NMDevice *device, + const char *specific_object, + GVariant *options, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data); +NMActiveConnection *nm_client_add_and_activate_connection_options_finish (NMClient *client, + GAsyncResult *result, + GError **error); + gboolean nm_client_deactivate_connection (NMClient *client, NMActiveConnection *active, GCancellable *cancellable, diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c index a542512742..235dc4aed1 100644 --- a/libnm/nm-manager.c +++ b/libnm/nm-manager.c @@ -1098,6 +1098,7 @@ nm_manager_add_and_activate_connection_async (NMManager *manager, NMConnection *partial, NMDevice *device, const char *specific_object, + GVariant *options, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data) @@ -1127,13 +1128,16 @@ nm_manager_add_and_activate_connection_async (NMManager *manager, dict = nm_connection_to_dbus (partial, NM_CONNECTION_SERIALIZE_ALL); if (!dict) dict = g_variant_new_array (G_VARIANT_TYPE ("{sa{sv}}"), NULL, 0); + if (!options) + options = g_variant_new_array (G_VARIANT_TYPE ("{sv}"), NULL, 0); - nmdbus_manager_call_add_and_activate_connection (priv->proxy, - dict, - nm_object_get_path (NM_OBJECT (device)), - specific_object ?: "/", - cancellable, - add_activate_cb, info); + nmdbus_manager_call_add_and_activate_connection2 (priv->proxy, + dict, + nm_object_get_path (NM_OBJECT (device)), + specific_object ?: "/", + options, + cancellable, + add_activate_cb, info); } NMActiveConnection * diff --git a/libnm/nm-manager.h b/libnm/nm-manager.h index 0a278aee58..26b7ef412a 100644 --- a/libnm/nm-manager.h +++ b/libnm/nm-manager.h @@ -163,6 +163,7 @@ void nm_manager_add_and_activate_connection_async (NMManager *ma NMConnection *partial, NMDevice *device, const char *specific_object, + GVariant *options, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index 9aa44a906e..76601d0172 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -1341,6 +1341,12 @@ class NetworkManager(ExportedObj): @dbus.service.method(dbus_interface=IFACE_NM, in_signature='a{sa{sv}}oo', out_signature='oo') def AddAndActivateConnection(self, con_hash, devpath, specific_object): + return self.AddAndActivateConnection2(con_hash, devpath, specific_object, dict()) + + @dbus.service.method(dbus_interface=IFACE_NM, in_signature='a{sa{sv}}ooa{sv}', out_signature='oo') + def AddAndActivateConnection2(self, con_hash, devpath, specific_object, options): + # TODO: Do some processing of the "options" parameter. + device = self.find_device_first(path = devpath, require = BusErr.UnknownDeviceException) conpath = gl.settings.AddConnection(con_hash) return (conpath, self.ActivateConnection(conpath, devpath, specific_object)) From 7842a58055eecaae2cef530f7383dc900bb49636 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 12:43:25 +0100 Subject: [PATCH 06/16] keep-alive: drop unused error argument --- src/nm-active-connection.c | 2 +- src/nm-keep-alive.c | 7 +++---- src/nm-keep-alive.h | 3 +-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 84c86ce5de..e14ac6e8b9 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1027,7 +1027,7 @@ nm_active_connection_bind_dbus_client (NMActiveConnection *self, GDBusConnection { NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); - nm_keep_alive_set_dbus_client_watch (priv->keep_alive, dbus_con, dbus_client, NULL); + nm_keep_alive_set_dbus_client_watch (priv->keep_alive, dbus_con, dbus_client); nm_keep_alive_sink (priv->keep_alive); } diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index be6dc2304b..8d0a197f41 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -175,10 +175,9 @@ name_owner_changed_cb (GDBusConnection *connection, } void -nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, - GDBusConnection *connection, - const char *client_address, - GError **error) +nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, + GDBusConnection *connection, + const char *client_address) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); diff --git a/src/nm-keep-alive.h b/src/nm-keep-alive.h index 5ca02a3c5a..b8c26e173d 100644 --- a/src/nm-keep-alive.h +++ b/src/nm-keep-alive.h @@ -50,7 +50,6 @@ void nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *s void nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, GDBusConnection *connection, - const char *client_address, - GError **error); + const char *client_address); #endif /* __NETWORKMANAGER_KEEP_ALIVE_H__ */ From 58923de4e4b1fab098707065a21f9055fb0a4602 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 12:46:14 +0100 Subject: [PATCH 07/16] keep-alive: various style fixes Some trivial changes: - move nm_keep_alive_new() after nm_keep_alive_init(), so that the functions that initialize the instance are beside each other. - prefer nm_streq*() over strcmp(). - wrap some lines. - remove some empty lines. --- src/nm-active-connection.c | 5 ++- src/nm-keep-alive.c | 76 +++++++++++++++++++++++--------------- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index e14ac6e8b9..ebe911e477 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1477,8 +1477,9 @@ constructed (GObject *object) g_return_if_fail (priv->subject); g_return_if_fail (priv->activation_reason != NM_ACTIVATION_REASON_UNSET); - if (priv->activation_reason == NM_ACTIVATION_REASON_AUTOCONNECT || - priv->activation_reason == NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES) { + if (NM_IN_SET ((NMActivationReason) priv->activation_reason, + NM_ACTIVATION_REASON_AUTOCONNECT, + NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES)) { nm_keep_alive_set_settings_connection_watch_visible (priv->keep_alive, priv->settings_connection.obj); nm_keep_alive_sink (priv->keep_alive); } diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index 8d0a197f41..890ef476b5 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -22,10 +22,11 @@ #include "nm-default.h" #include "nm-keep-alive.h" -#include "settings/nm-settings-connection.h" #include +#include "settings/nm-settings-connection.h" + /*****************************************************************************/ NM_GOBJECT_PROPERTIES_DEFINE (NMKeepAlive, @@ -33,13 +34,14 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMKeepAlive, ); typedef struct { - gboolean floating; - gboolean forced; NMSettingsConnection *connection; GDBusConnection *dbus_connection; char *dbus_client; guint subscription_id; + + bool floating:1; + bool forced:1; } NMKeepAlivePrivate; struct _NMKeepAlive { @@ -62,25 +64,18 @@ G_DEFINE_TYPE (NMKeepAlive, nm_keep_alive, G_TYPE_OBJECT) /*****************************************************************************/ -NMKeepAlive* nm_keep_alive_new (gboolean floating) -{ - NMKeepAlive *res = g_object_new (NM_TYPE_KEEP_ALIVE, NULL); - NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (res); - - priv->floating = floating; - - return res; -} - -gboolean nm_keep_alive_is_alive (NMKeepAlive *self) +gboolean +nm_keep_alive_is_alive (NMKeepAlive *self) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - if (priv->floating || priv->forced) + if ( priv->floating + || priv->forced) return TRUE; - if (priv->connection && NM_FLAGS_HAS (nm_settings_connection_get_flags (priv->connection), - NM_SETTINGS_CONNECTION_INT_FLAGS_VISIBLE)) + if ( priv->connection + && NM_FLAGS_HAS (nm_settings_connection_get_flags (priv->connection), + NM_SETTINGS_CONNECTION_INT_FLAGS_VISIBLE)) return TRUE; if (priv->dbus_client) @@ -89,7 +84,8 @@ gboolean nm_keep_alive_is_alive (NMKeepAlive *self) return FALSE; } -void nm_keep_alive_sink (NMKeepAlive *self) +void +nm_keep_alive_sink (NMKeepAlive *self) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); @@ -98,7 +94,8 @@ void nm_keep_alive_sink (NMKeepAlive *self) _notify (self, PROP_ALIVE); } -void nm_keep_alive_set_forced (NMKeepAlive *self, gboolean forced) +void +nm_keep_alive_set_forced (NMKeepAlive *self, gboolean forced) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); @@ -107,6 +104,8 @@ void nm_keep_alive_set_forced (NMKeepAlive *self, gboolean forced) _notify (self, PROP_ALIVE); } +/*****************************************************************************/ + static void connection_flags_changed (NMSettingsConnection *connection, NMKeepAlive *self) @@ -114,7 +113,6 @@ connection_flags_changed (NMSettingsConnection *connection, _notify (self, PROP_ALIVE); } - void nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, NMSettingsConnection *connection) @@ -133,6 +131,8 @@ nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, _notify (self, PROP_ALIVE); } +/*****************************************************************************/ + static void cleanup_dbus_watch (NMKeepAlive *self) { @@ -158,19 +158,16 @@ name_owner_changed_cb (GDBusConnection *connection, gpointer user_data) { NMKeepAlive *self = NM_KEEP_ALIVE (user_data); - const char *old_owner; const char *new_owner; g_variant_get (parameters, "(&s&s&s)", NULL, &old_owner, &new_owner); - if (g_strcmp0 (new_owner, "") != 0) + if (!nm_streq0 (new_owner, "")) return; _LOGD ("DBus client for keep alive disappeared from bus"); - cleanup_dbus_watch (self); - _notify (self, PROP_ALIVE); } @@ -190,13 +187,20 @@ nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, priv->dbus_client = g_strdup (client_address); priv->dbus_connection = g_object_ref (connection); - priv->subscription_id = - g_dbus_connection_signal_subscribe (connection, "org.freedesktop.DBus", - "org.freedesktop.DBus", "NameOwnerChanged", "/org/freedesktop/DBus", - priv->dbus_client, G_DBUS_SIGNAL_FLAGS_NONE, - name_owner_changed_cb, self, NULL); + priv->subscription_id = g_dbus_connection_signal_subscribe (connection, + "org.freedesktop.DBus", + "org.freedesktop.DBus", + "NameOwnerChanged", + "/org/freedesktop/DBus", + priv->dbus_client, + G_DBUS_SIGNAL_FLAGS_NONE, + name_owner_changed_cb, + self, + NULL); } +/*****************************************************************************/ + static void get_property (GObject *object, guint prop_id, @@ -215,10 +219,22 @@ get_property (GObject *object, } } +/*****************************************************************************/ + static void nm_keep_alive_init (NMKeepAlive *self) { - /* Nothing to do */ +} + +NMKeepAlive * +nm_keep_alive_new (gboolean floating) +{ + NMKeepAlive *res = g_object_new (NM_TYPE_KEEP_ALIVE, NULL); + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (res); + + priv->floating = floating; + + return res; } static void From be91d1cc4ef047a0a9f41711669f13d28f5fa906 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 13:07:22 +0100 Subject: [PATCH 08/16] keep-alive: cache the alive-state and only emit the signal when it changed The alive state is composed from various parts, let's only emit a _notify (self, PROP_ALIVE) when it actually changes. For that, cache the alive state and let _notify_alive() determine whether it changed and possibly emit the signal. --- src/nm-keep-alive.c | 54 ++++++++++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index 890ef476b5..4d67d907be 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -42,6 +42,7 @@ typedef struct { bool floating:1; bool forced:1; + bool alive:1; } NMKeepAlivePrivate; struct _NMKeepAlive { @@ -64,8 +65,8 @@ G_DEFINE_TYPE (NMKeepAlive, nm_keep_alive, G_TYPE_OBJECT) /*****************************************************************************/ -gboolean -nm_keep_alive_is_alive (NMKeepAlive *self) +static gboolean +_is_alive (NMKeepAlive *self) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); @@ -84,14 +85,34 @@ nm_keep_alive_is_alive (NMKeepAlive *self) return FALSE; } +static void +_notify_alive (NMKeepAlive *self) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + + if (priv->alive == _is_alive (self)) + return; + priv->alive = !priv->alive; + _notify (self, PROP_ALIVE); +} + +gboolean +nm_keep_alive_is_alive (NMKeepAlive *self) +{ + return NM_KEEP_ALIVE_GET_PRIVATE (self)->alive; +} + +/*****************************************************************************/ + void nm_keep_alive_sink (NMKeepAlive *self) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - priv->floating = FALSE; - - _notify (self, PROP_ALIVE); + if (priv->floating) { + priv->floating = FALSE; + _notify_alive (self); + } } void @@ -99,9 +120,10 @@ nm_keep_alive_set_forced (NMKeepAlive *self, gboolean forced) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - priv->forced = forced; - - _notify (self, PROP_ALIVE); + if (priv->forced != (!!forced)) { + priv->forced = forced; + _notify_alive (self); + } } /*****************************************************************************/ @@ -110,7 +132,7 @@ static void connection_flags_changed (NMSettingsConnection *connection, NMKeepAlive *self) { - _notify (self, PROP_ALIVE); + _notify_alive (self); } void @@ -128,7 +150,7 @@ nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, g_signal_connect_object (priv->connection, NM_SETTINGS_CONNECTION_FLAGS_CHANGED, G_CALLBACK (connection_flags_changed), self, 0); - _notify (self, PROP_ALIVE); + _notify_alive (self); } /*****************************************************************************/ @@ -168,7 +190,7 @@ name_owner_changed_cb (GDBusConnection *connection, _LOGD ("DBus client for keep alive disappeared from bus"); cleanup_dbus_watch (self); - _notify (self, PROP_ALIVE); + _notify_alive (self); } void @@ -224,17 +246,19 @@ get_property (GObject *object, static void nm_keep_alive_init (NMKeepAlive *self) { + nm_assert (NM_KEEP_ALIVE_GET_PRIVATE (self)->alive == _is_alive (self)); } NMKeepAlive * nm_keep_alive_new (gboolean floating) { - NMKeepAlive *res = g_object_new (NM_TYPE_KEEP_ALIVE, NULL); - NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (res); + NMKeepAlive *self = g_object_new (NM_TYPE_KEEP_ALIVE, NULL); + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); priv->floating = floating; - - return res; + priv->alive = TRUE; + nm_assert (priv->alive == _is_alive (self)); + return self; } static void From 3baf56b4741435b6092ec2b2d9a1c2704f685b95 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 12:56:14 +0100 Subject: [PATCH 09/16] keep-alive: cleanup tracking visible state of connection - in nm_keep_alive_set_settings_connection_watch_visible(), abort early when setting the same connection again (or %NULL again). - nm_keep_alive_set_settings_connection_watch_visible() would (already before) correctly disconnect the signal handler from the previous connection. As we anyway do explict disconnects, avoid the overhead of a weak pointer with g_signal_connect_object(). Just reuse the same setter to disconnect the signal in dispose(). - fix leaking priv->connection in nm_keep_alive_set_settings_connection_watch_visible(). - use g_signal_handlers_disconnect_by_func(), to be more specific about which signal we want to disconnect. --- src/nm-keep-alive.c | 47 ++++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index 4d67d907be..cfad13e6a3 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -135,22 +135,41 @@ connection_flags_changed (NMSettingsConnection *connection, _notify_alive (self); } +static void +_set_settings_connection_watch_visible (NMKeepAlive *self, + NMSettingsConnection *connection, + gboolean emit_signal) +{ + NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + gs_unref_object NMSettingsConnection *old_connection = NULL; + + if (priv->connection == connection) + return; + + if (priv->connection) { + g_signal_handlers_disconnect_by_func (priv->connection, + G_CALLBACK (connection_flags_changed), + self); + old_connection = g_steal_pointer (&priv->connection); + } + + if (connection) { + priv->connection = g_object_ref (connection); + g_signal_connect (priv->connection, + NM_SETTINGS_CONNECTION_FLAGS_CHANGED, + G_CALLBACK (connection_flags_changed), + self); + } + + if (emit_signal) + _notify_alive (self); +} + void nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, NMSettingsConnection *connection) { - NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - - if (priv->connection) { - g_signal_handlers_disconnect_by_data (priv->connection, self); - priv->connection = NULL; - } - - priv->connection = g_object_ref (connection); - g_signal_connect_object (priv->connection, NM_SETTINGS_CONNECTION_FLAGS_CHANGED, - G_CALLBACK (connection_flags_changed), self, 0); - - _notify_alive (self); + _set_settings_connection_watch_visible (self, connection, TRUE); } /*****************************************************************************/ @@ -265,10 +284,8 @@ static void dispose (GObject *object) { NMKeepAlive *self = NM_KEEP_ALIVE (object); - NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - - g_clear_object (&priv->connection); + _set_settings_connection_watch_visible (self, NULL, FALSE); cleanup_dbus_watch (self); } From 6b79af28d66a709a8feda9dc3824ac544db0e7bb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 13:29:40 +0100 Subject: [PATCH 10/16] keep-alive: minor cleanup of nm_keep_alive_set_dbus_client_watch() - always issue a _notify_alive(), just to be sure. At least in case where we clear a dbus-client watch, the alive state could change. - avoid the logging in cleanup_dbus_watch(), if there is nothing to cleanup. --- src/nm-keep-alive.c | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index cfad13e6a3..71cd43415c 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -179,14 +179,17 @@ cleanup_dbus_watch (NMKeepAlive *self) { NMKeepAlivePrivate *priv = NM_KEEP_ALIVE_GET_PRIVATE (self); + if (!priv->dbus_client) + return; + _LOGD ("Cleanup DBus client watch"); - g_clear_pointer (&priv->dbus_client, g_free); - - if (priv->dbus_connection) + nm_clear_g_free (&priv->dbus_client); + if (priv->dbus_connection) { g_dbus_connection_signal_unsubscribe (priv->dbus_connection, priv->subscription_id); - g_clear_object (&priv->dbus_connection); + g_clear_object (&priv->dbus_connection); + } } static void @@ -221,23 +224,26 @@ nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, cleanup_dbus_watch (self); - if (client_address == NULL) - return; + if (client_address) { + _LOGD ("Registering dbus client watch for keep alive"); - _LOGD ("Registering dbus client watch for keep alive"); + priv->dbus_client = g_strdup (client_address); + priv->dbus_connection = g_object_ref (connection); + priv->subscription_id = g_dbus_connection_signal_subscribe (connection, + "org.freedesktop.DBus", + "org.freedesktop.DBus", + "NameOwnerChanged", + "/org/freedesktop/DBus", + priv->dbus_client, + G_DBUS_SIGNAL_FLAGS_NONE, + name_owner_changed_cb, + self, + NULL); + /* FIXME: is there are race here and is it possible that name-owner is already gone? + * Do we need a GetNameOwner first? */ + } - priv->dbus_client = g_strdup (client_address); - priv->dbus_connection = g_object_ref (connection); - priv->subscription_id = g_dbus_connection_signal_subscribe (connection, - "org.freedesktop.DBus", - "org.freedesktop.DBus", - "NameOwnerChanged", - "/org/freedesktop/DBus", - priv->dbus_client, - G_DBUS_SIGNAL_FLAGS_NONE, - name_owner_changed_cb, - self, - NULL); + _notify_alive (self); } /*****************************************************************************/ From f10f0199826ea8b7b818bfe5e8268b4f4839a493 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 13:35:02 +0100 Subject: [PATCH 11/16] policy: don't check for valid error in active_connection_keep_alive_changed() Most (not all) functions that can fail and report the reason with an GError are required to set the error if they fail. It's a bug to claim to fail without returning the GError reason. Hence, our callers usually don't check whether a GError is present but just access it. Likewise, for better or worse, our GError codes are often not meaningful (unless explicitly documented). Meaning, logging the error code number is not helpful. Instead, error messages should be written in a manner that one can find the source code location where it happened. Also, return-early to reduce the indentation level of the code. Also, drop the code comment. It seems to just describe what is obviously visible by reading the source. It doesn't explain much beside that the "doesn't have a reason", but not really why. --- src/nm-policy.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 323df112de..5199afbf1c 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -2190,22 +2190,20 @@ active_connection_keep_alive_changed (NMActiveConnection *ac, NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); GError *error = NULL; - /* The connection does not have a reason to stay alive anymore. */ - if (!nm_active_connection_get_keep_alive (ac)) { - if (nm_active_connection_get_state (ac) <= NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { - if (!nm_manager_deactivate_connection (priv->manager, - ac, - NM_DEVICE_STATE_REASON_CONNECTION_REMOVED, - &error)) { - _LOGW (LOGD_DEVICE, "connection '%s' is no longer kept alive, but error deactivating it: (%d) %s", - nm_active_connection_get_settings_connection_id (ac), - error ? error->code : -1, - error ? error->message : "(unknown)"); - g_clear_error (&error); - } + if (nm_active_connection_get_keep_alive (ac)) + return; + + if (nm_active_connection_get_state (ac) <= NM_ACTIVE_CONNECTION_STATE_ACTIVATED) { + if (!nm_manager_deactivate_connection (priv->manager, + ac, + NM_DEVICE_STATE_REASON_CONNECTION_REMOVED, + &error)) { + _LOGW (LOGD_DEVICE, "connection '%s' is no longer kept alive, but error deactivating it: %s", + nm_active_connection_get_settings_connection_id (ac), + error->message); + g_clear_error (&error); } } - } static void From 6f28f4b661e8938c42951a8135bc0d6710978dfb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 18 Nov 2018 11:48:44 +0100 Subject: [PATCH 12/16] manager: allow add-and-activate option "bind" with non-volatile profiles For one, there was a bug here: we cannot "goto error" without setting the @error variable. Anyway, restricting "bind" "dbus-client" only to profiles that are "persist" mode "volatile" seems wrong. The "bind" option as it is, limits the lifetime of the active-connection. This has no direct relation with the lifetime of the setting-connection. Indeed, if the settings-connection's lifetime is itself set to "volatile", then it will indeed go away with the active-connection. However, these two concepts are not strictly related. In the future, we might add an option to limite the lifetime of a settings-connection to a D-Bus client ("bind-setting"). Possibly we should thus rename "bind" to "bind-activation", to make the distinction clearer. --- introspection/org.freedesktop.NetworkManager.xml | 2 +- src/nm-manager.c | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index 715e182eda..f78a989a4b 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -129,7 +129,7 @@ parameters. At this time the following options are supported: * persist: A string value of either "disk" (default), "memory" or "volatile". If "memory" is passed, the connection will not be saved to disk. If "volatile" is passed, the connection will not be saved to disk and will be destroyed when disconnected. - * bind: Bind the connections lifetime. Set to "dbus-name" to automatically disconnect when the requesting process disappears from the bus. The default of "none" means the connection is kept alive normally. If restricted, then persist must be set to "volatile". + * bind: Bind the activation lifetime. Set to "dbus-name" to automatically disconnect when the requesting process disappears from the bus. The default of "none" means the connection is kept activated normally. --> diff --git a/src/nm-manager.c b/src/nm-manager.c index e5a0964192..e7ea86ffa1 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5364,12 +5364,8 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!active) goto error; - if (g_strcmp0 (bind_lifetime, "dbus-client") == 0) { - if (persist != NM_SETTINGS_CONNECTION_PERSIST_MODE_VOLATILE_ONLY) - goto error; - + if (g_strcmp0 (bind_lifetime, "dbus-client") == 0) nm_active_connection_bind_dbus_client (active, dbus_connection, sender); - } nm_active_connection_authorize (active, incompl_conn, From bc23dc8ff007341a6e8f5e569b1454750f988ea7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 18 Nov 2018 11:52:13 +0100 Subject: [PATCH 13/16] manager: fix checking for bind-lifetime setting in add-and-activate Previously, @bind_lifetime was a string. While parsing the @options, we would set the string to the content of the parsed GVariant. Note that the GVariant is unrefed before we access @bind_lifetime, thus it's not guaranteed that it will still exist. Arguably, the string GVariant's lifetime is tied to the @options dictionary, so indeed it lives long enough. But that is not-obviously the case. Fix that by using a boolean instead. Also, rename @bind_lifetime to @bind_dbus_client. --- src/nm-manager.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index e7ea86ffa1..c0bb1c10a5 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5235,7 +5235,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, const char *specific_object_path; gs_free NMConnection **conns = NULL; NMSettingsConnectionPersistMode persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; - const char *bind_lifetime = "none"; + gboolean bind_dbus_client = FALSE; if (g_strcmp0 (method_info->parent.name, "AddAndActivateConnection2") == 0) g_variant_get (parameters, "(@a{sa{sv}}&o&o@a{sv})", &settings, &device_path, &specific_object_path, &options); @@ -5275,15 +5275,16 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { s = g_variant_get_string (option_value, NULL); - if (!NM_IN_STRSET (s, "dbus-client", "none")) { + if (nm_streq (s, "dbus-client")) + bind_dbus_client = TRUE; + else if (nm_streq (s, "none")) + bind_dbus_client = FALSE; + else { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, "Option \"bind\" must be one of \"dbus-client\" or \"none\"."); goto error; } - - bind_lifetime = s; - } else { /* Unknown argument */ error = g_error_new_literal (NM_MANAGER_ERROR, @@ -5364,7 +5365,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!active) goto error; - if (g_strcmp0 (bind_lifetime, "dbus-client") == 0) + if (bind_dbus_client) nm_active_connection_bind_dbus_client (active, dbus_connection, sender); nm_active_connection_authorize (active, From 28c386df8a7c2542c38cea54cdcd75d8f11b0b52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 17 Nov 2018 15:54:49 +0100 Subject: [PATCH 14/16] manager: prefer nm_streq over strcmp in impl_manager_add_and_activate_connection() - use nm_streq() instead of g_strcmp0(). I think streq() is easier to understand. - the strings that are checked here must never be %NULL, because they come from string variants. Use nm_streq() instead of nm_streq0() or g_strcmp0(). - don't add a "." to the GError messages. GError messages are commonly embedded in a larger message, and shoult not themself contain the dot. --- src/nm-manager.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index c0bb1c10a5..c960155fde 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5254,25 +5254,24 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, option_value_free = option_value; - if ((g_strcmp0 (option_name, "persist") == 0) && - g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { + if ( nm_streq (option_name, "persist") + && g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { s = g_variant_get_string (option_value, NULL); - if (g_strcmp0 (s, "volatile") == 0) { + if (nm_streq (s, "volatile")) persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_VOLATILE_ONLY; - } else if (g_strcmp0 (s, "memory") == 0) { + else if (nm_streq (s, "memory")) persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY; - } else if (g_strcmp0 (s, "disk") == 0) { + else if (nm_streq (s, "disk")) persist = NM_SETTINGS_CONNECTION_PERSIST_MODE_DISK; - } else { + else { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "Option \"persist\" must be one of \"volatile\", \"memory\" or \"disk\"."); + "Option \"persist\" must be one of \"volatile\", \"memory\" or \"disk\""); goto error; } - - } else if ((g_strcmp0 (option_name, "bind") == 0) && - g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { + } else if ( nm_streq (option_name, "bind") + && g_variant_is_of_type (option_value, G_VARIANT_TYPE_STRING)) { s = g_variant_get_string (option_value, NULL); if (nm_streq (s, "dbus-client")) @@ -5282,14 +5281,13 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, else { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "Option \"bind\" must be one of \"dbus-client\" or \"none\"."); + "Option \"bind\" must be one of \"dbus-client\" or \"none\""); goto error; } } else { - /* Unknown argument */ error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "Unknown extra option passed."); + "Unknown extra option passed"); goto error; } } From 1d9a808a58046d1ff744940e1df9dc6215eb1d05 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 18 Nov 2018 17:59:31 +0100 Subject: [PATCH 15/16] libnm: add missing NM_AVAILABLE_IN_1_16 macros for new API --- libnm/nm-client.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libnm/nm-client.h b/libnm/nm-client.h index dc5649f3cf..98527cde9c 100644 --- a/libnm/nm-client.h +++ b/libnm/nm-client.h @@ -339,6 +339,7 @@ NMActiveConnection *nm_client_add_and_activate_connection_finish (NMClient *clie GAsyncResult *result, GError **error); +NM_AVAILABLE_IN_1_16 void nm_client_add_and_activate_connection_options_async (NMClient *client, NMConnection *partial, NMDevice *device, @@ -347,6 +348,7 @@ void nm_client_add_and_activate_connection_options_async (NMClie GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); +NM_AVAILABLE_IN_1_16 NMActiveConnection *nm_client_add_and_activate_connection_options_finish (NMClient *client, GAsyncResult *result, GError **error); From 26eaca89b84082d25e1b73a4c6c2c1fffdf26f02 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 19 Nov 2018 10:53:30 +0100 Subject: [PATCH 16/16] libnm: drop "_async" suffix from nm_client_add_and_activate_connection_options() Synchronous D-Bus calls seems harmful to me, such API should not be added to libnm. As such, all API is by default and preferably "_async". Don't add an "_async" suffix. While we are not consistent in libnm about this, I think for new code we should. --- libnm/libnm.ver | 2 +- libnm/nm-client.c | 22 +++++++++++----------- libnm/nm-client.h | 16 ++++++++-------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 269f406cd3..09681e4733 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1447,7 +1447,7 @@ global: libnm_1_16_0 { global: - nm_client_add_and_activate_connection_options_async; + nm_client_add_and_activate_connection_options; nm_client_add_and_activate_connection_options_finish; nm_device_get_connectivity; } libnm_1_14_0; diff --git a/libnm/nm-client.c b/libnm/nm-client.c index 576fbacbc4..d37feb100f 100644 --- a/libnm/nm-client.c +++ b/libnm/nm-client.c @@ -1269,7 +1269,7 @@ nm_client_add_and_activate_connection_finish (NMClient *client, } /** - * nm_client_add_and_activate_connection_options_async: + * nm_client_add_and_activate_connection_options: * @client: a #NMClient * @partial: (allow-none): an #NMConnection to add; the connection may be * partially filled (or even %NULL) and will be completed by NetworkManager @@ -1314,14 +1314,14 @@ nm_client_add_and_activate_connection_finish (NMClient *client, * Since: 1.16 **/ void -nm_client_add_and_activate_connection_options_async (NMClient *client, - NMConnection *partial, - NMDevice *device, - const char *specific_object, - GVariant *options, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data) +nm_client_add_and_activate_connection_options (NMClient *client, + NMConnection *partial, + NMDevice *device, + const char *specific_object, + GVariant *options, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { GSimpleAsyncResult *simple; GError *error = NULL; @@ -1337,7 +1337,7 @@ nm_client_add_and_activate_connection_options_async (NMClient *client, } simple = g_simple_async_result_new (G_OBJECT (client), callback, user_data, - nm_client_add_and_activate_connection_options_async); + nm_client_add_and_activate_connection_options); if (cancellable) g_simple_async_result_set_check_cancellable (simple, cancellable); nm_manager_add_and_activate_connection_async (NM_CLIENT_GET_PRIVATE (client)->manager, @@ -1351,7 +1351,7 @@ nm_client_add_and_activate_connection_options_async (NMClient *client, * @result: the result passed to the #GAsyncReadyCallback * @error: location for a #GError, or %NULL * - * Gets the result of a call to nm_client_add_and_activate_connection_options_async(). + * Gets the result of a call to nm_client_add_and_activate_connection_options(). * * You can call nm_active_connection_get_connection() on the returned * #NMActiveConnection to find the path of the created #NMConnection. diff --git a/libnm/nm-client.h b/libnm/nm-client.h index 98527cde9c..59567c1737 100644 --- a/libnm/nm-client.h +++ b/libnm/nm-client.h @@ -340,14 +340,14 @@ NMActiveConnection *nm_client_add_and_activate_connection_finish (NMClient *clie GError **error); NM_AVAILABLE_IN_1_16 -void nm_client_add_and_activate_connection_options_async (NMClient *client, - NMConnection *partial, - NMDevice *device, - const char *specific_object, - GVariant *options, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data); +void nm_client_add_and_activate_connection_options (NMClient *client, + NMConnection *partial, + NMDevice *device, + const char *specific_object, + GVariant *options, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data); NM_AVAILABLE_IN_1_16 NMActiveConnection *nm_client_add_and_activate_connection_options_finish (NMClient *client, GAsyncResult *result,