From f95a5263667a03aaf67635d28532c242d3025bd9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Nov 2018 14:51:22 +0100 Subject: [PATCH] keep-alive: use NMKeepAlive API directly instead of via NMActiveConnection NMKeepAlive is a proper GObject type, with a specific API that on the one end allows to configure watches/bindings, and on the other end exposes and is-alive property and the owner instance. That's great, as NMActiveConnection is not concerned with either end (moving complexity away from "nm-active-connection.c") and as we later can reuse NMKeepAlive with NMSettingsConnection. However, we don't need to wrap this API by NMActiveConnection. Doing so means, we need to expose all the watch/bind functions also as part of NMActiveConnection API. The only ugliness here is, that NMPolicy subscribes to property changed signal of the keep alive instance, which would fail horribly if NMActiveConnection ever decides to swap the keep alive instance (in which case NMPolicy would have to disconnect the signal, and possibly reconnect it to another NMKeepAlive instance). We avoid that by just not doing that and documenting it. --- src/nm-active-connection.c | 63 +++++++++++++------------------------- src/nm-active-connection.h | 7 +---- src/nm-manager.c | 10 ++++-- src/nm-policy.c | 47 ++++++++++++++++++---------- 4 files changed, 62 insertions(+), 65 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 559d05cf13..6d504e0e4d 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -105,7 +105,6 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMActiveConnection, PROP_INT_MASTER_READY, PROP_INT_ACTIVATION_TYPE, PROP_INT_ACTIVATION_REASON, - PROP_INT_KEEP_ALIVE, ); enum { @@ -176,14 +175,6 @@ 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, @@ -921,12 +912,30 @@ nm_active_connection_get_activation_reason (NMActiveConnection *self) return NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->activation_reason; } -gboolean +/*****************************************************************************/ + +/** + * nm_active_connection_get_keep_alive: + * @self: the #NMActiveConnection instance + * + * Gives the #NMKeepAlive instance of the active connection. Note that + * @self is guaranteed not to swap the keep-alive instance, so it is + * in particular safe to assume that the keep-alive instance is alive + * as long as @self, and that nm_active_connection_get_keep_alive() + * will return always the same instance. + * + * In particular this means, that it is safe and encouraged, that you + * register to the notify:alive property changed signal of the returned + * instance. + * + * Returns: the #NMKeepAlive instance. + */ +NMKeepAlive * nm_active_connection_get_keep_alive (NMActiveConnection *self) { - NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); + g_return_val_if_fail (NM_IS_ACTIVE_CONNECTION (self), NULL); - return nm_keep_alive_is_alive (priv->keep_alive); + return NM_ACTIVE_CONNECTION_GET_PRIVATE (self)->keep_alive; } /*****************************************************************************/ @@ -1020,23 +1029,6 @@ 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); - nm_keep_alive_arm (priv->keep_alive); -} - /*****************************************************************************/ static void @@ -1335,9 +1327,6 @@ 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; @@ -1446,12 +1435,9 @@ nm_active_connection_init (NMActiveConnection *self) priv->activation_type = NM_ACTIVATION_TYPE_MANAGED; priv->version_id = _version_id_new (); + /* the keep-alive instance must never change. Callers rely on that. */ priv->keep_alive = nm_keep_alive_new (); _nm_keep_alive_set_owner (priv->keep_alive, G_OBJECT (self)); - g_signal_connect_object (priv->keep_alive, "notify::" NM_KEEP_ALIVE_ALIVE, - (GCallback) keep_alive_alive_changed, - self, - G_CONNECT_SWAPPED); } static void @@ -1746,11 +1732,6 @@ 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 5b2e421507..2b5a6b59ee 100644 --- a/src/nm-active-connection.h +++ b/src/nm-active-connection.h @@ -59,7 +59,6 @@ #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" @@ -186,12 +185,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); +NMKeepAlive *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 4c9b11a524..b05fbb443a 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -39,6 +39,7 @@ #include "platform/nm-platform.h" #include "platform/nmp-object.h" #include "nm-hostname-manager.h" +#include "nm-keep-alive.h" #include "nm-rfkill-manager.h" #include "dhcp/nm-dhcp-manager.h" #include "settings/nm-settings.h" @@ -5378,8 +5379,13 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!active) goto error; - if (bind_dbus_client) - nm_active_connection_bind_dbus_client (active, dbus_connection, sender); + if (bind_dbus_client) { + NMKeepAlive *keep_alive; + + keep_alive = nm_active_connection_get_keep_alive (active); + nm_keep_alive_set_dbus_client_watch (keep_alive, dbus_connection, sender); + nm_keep_alive_arm (keep_alive); + } nm_active_connection_authorize (active, incompl_conn, diff --git a/src/nm-policy.c b/src/nm-policy.c index 4e47b8c902..e4facf2614 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -30,6 +30,7 @@ #include "NetworkManagerUtils.h" #include "nm-act-request.h" +#include "nm-keep-alive.h" #include "devices/nm-device.h" #include "nm-setting-ip4-config.h" #include "nm-setting-connection.h" @@ -2183,26 +2184,36 @@ active_connection_state_changed (NMActiveConnection *active, } static void -active_connection_keep_alive_changed (NMActiveConnection *ac, +active_connection_keep_alive_changed (NMKeepAlive *keep_alive, GParamSpec *pspec, NMPolicy *self) { - NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); + NMPolicyPrivate *priv; + NMActiveConnection *ac; GError *error = NULL; - if (nm_active_connection_get_keep_alive (ac)) + nm_assert (NM_IS_POLICY (self)); + nm_assert (NM_IS_KEEP_ALIVE (keep_alive)); + nm_assert (NM_IS_ACTIVE_CONNECTION (nm_keep_alive_get_owner (keep_alive))); + + if (nm_keep_alive_is_alive (keep_alive)) 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); - } + ac = nm_keep_alive_get_owner (keep_alive); + + if (nm_active_connection_get_state (ac) > NM_ACTIVE_CONNECTION_STATE_ACTIVATED) + return; + + priv = NM_POLICY_GET_PRIVATE (self); + + 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); } } @@ -2213,6 +2224,7 @@ active_connection_added (NMManager *manager, { NMPolicyPrivate *priv = user_data; NMPolicy *self = _PRIV_TO_SELF (priv); + NMKeepAlive *keep_alive; if (NM_IS_VPN_CONNECTION (active)) { g_signal_connect (active, NM_VPN_CONNECTION_INTERNAL_STATE_CHANGED, @@ -2223,13 +2235,16 @@ active_connection_added (NMManager *manager, self); } + keep_alive = nm_active_connection_get_keep_alive (active); + 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_signal_connect (keep_alive, + "notify::" NM_KEEP_ALIVE_ALIVE, G_CALLBACK (active_connection_keep_alive_changed), self); - active_connection_keep_alive_changed (active, NULL, self); + active_connection_keep_alive_changed (keep_alive, NULL, self); } static void @@ -2249,7 +2264,7 @@ active_connection_removed (NMManager *manager, g_signal_handlers_disconnect_by_func (active, active_connection_state_changed, self); - g_signal_handlers_disconnect_by_func (active, + g_signal_handlers_disconnect_by_func (nm_active_connection_get_keep_alive (active), active_connection_keep_alive_changed, self); }