From f09e7797d448f7fe5d9cee8c4e351ad5b3837770 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 May 2018 13:06:03 +0200 Subject: [PATCH 1/5] core: add nm_shutdown_register_watchdog() for marking object to wait for shutdown Eventually we should do a coordinated shutdown when NetworkManager exits. That is a large work, ensuring that all asynchronous actions are cancellable (in time), and that we wait for all pending operations to complete. Add a function nm_shutdown_register_watchdog(), so that objects can register themselves, to block shutdown until they are destroyed. It's not yet used, because actually iterating the mainloop during shutdown can only be done, once the code is prepared to be ready for that. But we already need the function, so that we can refactor individual parts of the code, in preparation of using it in the future. --- src/NetworkManagerUtils.c | 89 +++++++++++++++++++++++++++++++++++++++ src/NetworkManagerUtils.h | 12 ++++++ 2 files changed, 101 insertions(+) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 78e1ac19c3..a3bd601359 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -23,6 +23,8 @@ #include "NetworkManagerUtils.h" +#include "nm-utils/nm-c-list.h" + #include "nm-common-macros.h" #include "nm-utils.h" #include "nm-setting-connection.h" @@ -907,3 +909,90 @@ nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, return no_match_value; } +/*****************************************************************************/ + +struct _NMShutdownWaitObjHandle { + CList lst; + GObject *watched_obj; + const char *msg_reason; +}; + +static CList _shutdown_waitobj_lst_head; + +static void +_shutdown_waitobj_unregister (NMShutdownWaitObjHandle *handle) +{ + c_list_unlink_stale (&handle->lst); + g_slice_free (NMShutdownWaitObjHandle, handle); + + /* FIXME(shutdown): check whether the object list is empty, and + * signal shutdown-complete */ +} + +static void +_shutdown_waitobj_cb (gpointer user_data, + GObject *where_the_object_was) +{ + NMShutdownWaitObjHandle *handle = user_data; + + nm_assert (handle); + nm_assert (handle->watched_obj == where_the_object_was); + _shutdown_waitobj_unregister (handle); +} + +/** + * _nm_shutdown_wait_obj_register: + * @watched_obj: the object to watch. Takes a weak reference on the object + * to be notified when it gets destroyed. + * @msg_reason: a reason message, for debugging and logging purposes. It + * must be a static string. Or at least, be alive at least as long as + * @watched_obj. So, theoretically, if you need a dynamic @msg_reason, + * you could attach it to @watched_obj's user-data. + * + * Keep track of @watched_obj until it gets destroyed. During shutdown, + * we wait until all watched objects are destroyed. This is useful, if + * this object still conducts some asynchronous action, which needs to + * complete before NetworkManager is allowed to terminate. We re-use + * the reference-counter of @watched_obj as signal, that the object + * is still used. + * + * FIXME(shutdown): proper shutdown is not yet implemented, and registering + * an object (currently) has no effect. + * + * Returns: a handle to unregister the object. The caller may choose to ignore + * the handle, in which case, the object will be automatically unregistered, + * once it gets destroyed. + */ +NMShutdownWaitObjHandle * +_nm_shutdown_wait_obj_register (GObject *watched_obj, + const char *msg_reason) +{ + NMShutdownWaitObjHandle *handle; + + g_return_val_if_fail (G_IS_OBJECT (watched_obj), NULL); + + if (G_UNLIKELY (!_shutdown_waitobj_lst_head.next)) + c_list_init (&_shutdown_waitobj_lst_head); + + handle = g_slice_new (NMShutdownWaitObjHandle); + handle->watched_obj = watched_obj; + /* we don't clone the string. We require the caller to use pass a static message. + * If he really cannot do that, he should attach the string to the watched_obj + * as user-data. */ + handle->msg_reason = msg_reason; + c_list_link_tail (&_shutdown_waitobj_lst_head, &handle->lst); + g_object_weak_ref (watched_obj, _shutdown_waitobj_cb, handle); + return handle; +} + +void +nm_shutdown_wait_obj_unregister (NMShutdownWaitObjHandle *handle) +{ + g_return_if_fail (handle); + + nm_assert (G_IS_OBJECT (handle->watched_obj)); + nm_assert (nm_c_list_contains_entry (&_shutdown_waitobj_lst_head, handle, lst)); + + g_object_weak_unref (handle->watched_obj, _shutdown_waitobj_cb, handle); + _shutdown_waitobj_unregister (handle); +} diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index 13bdb67e85..b8d3a4f064 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -53,6 +53,18 @@ int nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, const GSList *specs, int no_match_value); + +/*****************************************************************************/ + +typedef struct _NMShutdownWaitObjHandle NMShutdownWaitObjHandle; + +NMShutdownWaitObjHandle *_nm_shutdown_wait_obj_register (GObject *watched_obj, + const char *msg_reason); + +#define nm_shutdown_wait_obj_register(watched_obj, msg_reason) _nm_shutdown_wait_obj_register((watched_obj), (""msg_reason"")) + +void nm_shutdown_wait_obj_unregister (NMShutdownWaitObjHandle *handle); + /*****************************************************************************/ #endif /* __NETWORKMANAGER_UTILS_H__ */ From 515663519fbefcc86b7965dacd59e2f1dd10a9ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 May 2018 10:02:42 +0200 Subject: [PATCH 2/5] ppp-manager: refactor killing pppd process by using _ppp_kill() function - add callback arguments to _ppp_kill(). Although most callers don't care, it makes it more obvious that this kills the process asynchronously. - the call to nm_utils_kill_child_async() is complicated, with many arguments. Only call it from one place, and re-use the simpler wrapper function _ppp_kill() everywhere. --- src/ppp/nm-ppp-manager.c | 49 ++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c index 7d1eb4089d..40fd8e1d87 100644 --- a/src/ppp/nm-ppp-manager.c +++ b/src/ppp/nm-ppp-manager.c @@ -135,7 +135,9 @@ G_DEFINE_TYPE (NMPPPManager, nm_ppp_manager, NM_TYPE_DBUS_OBJECT) /*****************************************************************************/ static void _ppp_cleanup (NMPPPManager *manager); -static void _ppp_kill (NMPPPManager *manager); +static gboolean _ppp_kill (NMPPPManager *manager, + NMUtilsKillChildAsyncCb callback, + void *user_data); /*****************************************************************************/ @@ -781,7 +783,7 @@ pppd_timed_out (gpointer data) _LOGW ("pppd timed out or didn't initialize our dbus module"); _ppp_cleanup (manager); - _ppp_kill (manager); + _ppp_kill (manager, NULL, NULL); g_signal_emit (manager, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); @@ -1121,19 +1123,23 @@ out: return priv->pid > 0; } -static void -_ppp_kill (NMPPPManager *manager) +static gboolean +_ppp_kill (NMPPPManager *manager, + NMUtilsKillChildAsyncCb callback, + void *user_data) { - NMPPPManagerPrivate *priv; + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); - g_return_if_fail (NM_IS_PPP_MANAGER (manager)); - - priv = NM_PPP_MANAGER_GET_PRIVATE (manager); - - if (priv->pid) { - nm_utils_kill_child_async (priv->pid, SIGTERM, LOGD_PPP, "pppd", 2000, NULL, NULL); - priv->pid = 0; + if (!priv->pid) { + /* not PID. Signal that there was nothing to kill, which consequently + * implies that the callback will not be invoked. */ + return FALSE; } + + nm_utils_kill_child_async (nm_steal_int (&priv->pid), + SIGTERM, LOGD_PPP, "pppd", 2000, + callback, user_data); + return TRUE; } static void @@ -1204,8 +1210,10 @@ static void kill_child_ready (pid_t pid, gboolean success, int child_status, - StopContext *ctx) + gpointer user_data) { + StopContext *ctx = user_data; + if (stop_context_complete_if_cancelled (ctx)) return; stop_context_complete (ctx); @@ -1243,15 +1251,8 @@ _ppp_manager_stop_async (NMPPPManager *manager, return; } - /* No cancellable operation, so just wait until it returns always */ - nm_utils_kill_child_async (priv->pid, - SIGTERM, - LOGD_PPP, - "pppd", - 2000, - (NMUtilsKillChildAsyncCb) kill_child_ready, - ctx); - priv->pid = 0; + if (!_ppp_kill (manager, kill_child_ready, ctx)) + nm_assert_not_reached (); } static void @@ -1263,7 +1264,7 @@ _ppp_manager_stop_sync (NMPPPManager *manager) nm_dbus_object_unexport (dbus); _ppp_cleanup (manager); - _ppp_kill (manager); + _ppp_kill (manager, NULL, NULL); } /*****************************************************************************/ @@ -1337,7 +1338,7 @@ dispose (GObject *object) nm_dbus_object_unexport (dbus); _ppp_cleanup (self); - _ppp_kill (self); + _ppp_kill (self, NULL, NULL); g_clear_object (&priv->act_req); From 53d04a1dfad0655a7484a506b060da3e36acfe4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 May 2018 10:08:55 +0200 Subject: [PATCH 3/5] ppp-manager/trivial: rename variables holding self pointers We usually structure our code in a (pseudo) object oriented way. It makes sense to call the variable for the target object "self", it is more familiar and usually done. --- src/ppp/nm-ppp-manager.c | 128 +++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 64 deletions(-) diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c index 40fd8e1d87..a91e5cb3c5 100644 --- a/src/ppp/nm-ppp-manager.c +++ b/src/ppp/nm-ppp-manager.c @@ -134,8 +134,8 @@ G_DEFINE_TYPE (NMPPPManager, nm_ppp_manager, NM_TYPE_DBUS_OBJECT) /*****************************************************************************/ -static void _ppp_cleanup (NMPPPManager *manager); -static gboolean _ppp_kill (NMPPPManager *manager, +static void _ppp_cleanup (NMPPPManager *self); +static gboolean _ppp_kill (NMPPPManager *self, NMUtilsKillChildAsyncCb callback, void *user_data); @@ -175,8 +175,8 @@ _ppp_manager_set_route_parameters (NMPPPManager *self, static gboolean monitor_cb (gpointer user_data) { - NMPPPManager *manager = NM_PPP_MANAGER (user_data); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (user_data); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); const char *ifname; ifname = nm_platform_link_get_name (NM_PLATFORM_GET, priv->ifindex); @@ -192,7 +192,7 @@ monitor_cb (gpointer user_data) if (errno != ENODEV) _LOGW ("could not read ppp stats: %s", strerror (errno)); } else { - g_signal_emit (manager, signals[STATS], 0, + g_signal_emit (self, signals[STATS], 0, (guint) stats.p.ppp_ibytes, (guint) stats.p.ppp_obytes); } @@ -202,9 +202,9 @@ monitor_cb (gpointer user_data) } static void -monitor_stats (NMPPPManager *manager) +monitor_stats (NMPPPManager *self) { - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); /* already monitoring */ if (priv->monitor_fd >= 0) @@ -215,7 +215,7 @@ monitor_stats (NMPPPManager *manager) g_warn_if_fail (priv->monitor_id == 0); if (priv->monitor_id) g_source_remove (priv->monitor_id); - priv->monitor_id = g_timeout_add_seconds (5, monitor_cb, manager); + priv->monitor_id = g_timeout_add_seconds (5, monitor_cb, self); } else _LOGW ("could not monitor PPP stats: %s", strerror (errno)); } @@ -353,8 +353,8 @@ impl_ppp_manager_need_secrets (NMDBusObject *obj, GDBusMethodInvocation *invocation, GVariant *parameters) { - NMPPPManager *manager = NM_PPP_MANAGER (obj); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (obj); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); NMConnection *applied_connection; const char *username = NULL; const char *password = NULL; @@ -373,7 +373,7 @@ impl_ppp_manager_need_secrets (NMDBusObject *obj, if (extract_details_from_connection (applied_connection, NULL, &username, &password, &error)) { /* Send existing secrets to the PPP plugin */ priv->pending_secrets_context = invocation; - ppp_secrets_cb (priv->act_req, priv->secrets_id, NULL, NULL, manager); + ppp_secrets_cb (priv->act_req, priv->secrets_id, NULL, NULL, self); } else { _LOGW ("%s", error->message); g_dbus_method_invocation_take_error (priv->pending_secrets_context, error); @@ -395,7 +395,7 @@ impl_ppp_manager_need_secrets (NMDBusObject *obj, flags, hints ? g_ptr_array_index (hints, 0) : NULL, ppp_secrets_cb, - manager); + self); g_object_set_qdata (G_OBJECT (applied_connection), ppp_manager_secret_tries_quark (), GUINT_TO_POINTER (++tries)); priv->pending_secrets_context = invocation; @@ -412,11 +412,11 @@ impl_ppp_manager_set_state (NMDBusObject *obj, GDBusMethodInvocation *invocation, GVariant *parameters) { - NMPPPManager *manager = NM_PPP_MANAGER (obj); + NMPPPManager *self = NM_PPP_MANAGER (obj); guint32 state; g_variant_get (parameters, "(u)", &state); - g_signal_emit (manager, signals[STATE_CHANGED], 0, (guint) state); + g_signal_emit (self, signals[STATE_CHANGED], 0, (guint) state); g_dbus_method_invocation_return_value (invocation, NULL); } @@ -429,8 +429,8 @@ impl_ppp_manager_set_ifindex (NMDBusObject *obj, GDBusMethodInvocation *invocation, GVariant *parameters) { - NMPPPManager *manager = NM_PPP_MANAGER (obj); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (obj); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); const NMPlatformLink *plink = NULL; nm_auto_nmpobj const NMPObject *obj_keep_alive = NULL; gint32 ifindex; @@ -461,7 +461,7 @@ impl_ppp_manager_set_ifindex (NMDBusObject *obj, obj_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink)); - g_signal_emit (manager, signals[IFINDEX_SET], 0, ifindex, plink->name); + g_signal_emit (self, signals[IFINDEX_SET], 0, ifindex, plink->name); g_dbus_method_invocation_return_value (invocation, NULL); } @@ -500,8 +500,8 @@ impl_ppp_manager_set_ip4_config (NMDBusObject *obj, GDBusMethodInvocation *invocation, GVariant *parameters) { - NMPPPManager *manager = NM_PPP_MANAGER (obj); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (obj); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); gs_unref_object NMIP4Config *config = NULL; NMPlatformIP4Address address; guint32 u32, mtu; @@ -514,7 +514,7 @@ impl_ppp_manager_set_ip4_config (NMDBusObject *obj, nm_clear_g_source (&priv->ppp_timeout_handler); - if (!set_ip_config_common (manager, config_dict, &mtu)) + if (!set_ip_config_common (self, config_dict, &mtu)) goto out; config = nm_ip4_config_new (nm_platform_get_multi_idx (NM_PLATFORM_GET), priv->ifindex); @@ -566,7 +566,7 @@ impl_ppp_manager_set_ip4_config (NMDBusObject *obj, } /* Push the IP4 config up to the device */ - g_signal_emit (manager, signals[IP4_CONFIG], 0, config); + g_signal_emit (self, signals[IP4_CONFIG], 0, config); out: g_dbus_method_invocation_return_value (invocation, NULL); @@ -610,8 +610,8 @@ impl_ppp_manager_set_ip6_config (NMDBusObject *obj, GDBusMethodInvocation *invocation, GVariant *parameters) { - NMPPPManager *manager = NM_PPP_MANAGER (obj); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (obj); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); gs_unref_object NMIP6Config *config = NULL; NMPlatformIP6Address addr; struct in6_addr a; @@ -625,7 +625,7 @@ impl_ppp_manager_set_ip6_config (NMDBusObject *obj, nm_clear_g_source (&priv->ppp_timeout_handler); - if (!set_ip_config_common (manager, config_dict, NULL)) + if (!set_ip_config_common (self, config_dict, NULL)) goto out; config = nm_ip6_config_new (nm_platform_get_multi_idx (NM_PLATFORM_GET), priv->ifindex); @@ -653,7 +653,7 @@ impl_ppp_manager_set_ip6_config (NMDBusObject *obj, nm_ip6_config_add_address (config, &addr); /* Push the IPv6 config and interface identifier up to the device */ - g_signal_emit (manager, signals[IP6_CONFIG], 0, &iid, config); + g_signal_emit (self, signals[IP6_CONFIG], 0, &iid, config); } else _LOGE ("invalid IPv6 address received!"); @@ -746,8 +746,8 @@ NM_UTILS_LOOKUP_STR_DEFINE_STATIC (pppd_exit_code_to_str, int, static void ppp_watch_cb (GPid pid, int status, gpointer user_data) { - NMPPPManager *manager = NM_PPP_MANAGER (user_data); - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManager *self = NM_PPP_MANAGER (user_data); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); int err; const long long lpid = (long long) pid; @@ -772,20 +772,20 @@ ppp_watch_cb (GPid pid, int status, gpointer user_data) priv->pid = 0; priv->ppp_watch_id = 0; - _ppp_cleanup (manager); - g_signal_emit (manager, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); + _ppp_cleanup (self); + g_signal_emit (self, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); } static gboolean pppd_timed_out (gpointer data) { - NMPPPManager *manager = NM_PPP_MANAGER (data); + NMPPPManager *self = NM_PPP_MANAGER (data); _LOGW ("pppd timed out or didn't initialize our dbus module"); - _ppp_cleanup (manager); - _ppp_kill (manager, NULL, NULL); + _ppp_cleanup (self); + _ppp_kill (self, NULL, NULL); - g_signal_emit (manager, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); + g_signal_emit (self, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); return FALSE; } @@ -1010,7 +1010,7 @@ pppoe_fill_defaults (NMSettingPpp *setting) } static gboolean -_ppp_manager_start (NMPPPManager *manager, +_ppp_manager_start (NMPPPManager *self, NMActRequest *req, const char *ppp_name, guint32 timeout_secs, @@ -1030,10 +1030,10 @@ _ppp_manager_start (NMPPPManager *manager, gboolean ip6_enabled = FALSE; gboolean ip4_enabled = FALSE; - g_return_val_if_fail (NM_IS_PPP_MANAGER (manager), FALSE); + g_return_val_if_fail (NM_IS_PPP_MANAGER (self), FALSE); g_return_val_if_fail (NM_IS_ACT_REQUEST (req), FALSE); - priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + priv = NM_PPP_MANAGER_GET_PRIVATE (self); #if !WITH_PPP /* PPP support disabled */ @@ -1044,7 +1044,7 @@ _ppp_manager_start (NMPPPManager *manager, return FALSE; #endif - nm_dbus_object_export (NM_DBUS_OBJECT (manager)); + nm_dbus_object_export (NM_DBUS_OBJECT (self)); priv->pid = 0; @@ -1079,7 +1079,7 @@ _ppp_manager_start (NMPPPManager *manager, ip6_method = nm_utils_get_ip_config_method (connection, NM_TYPE_SETTING_IP6_CONFIG); ip6_enabled = g_strcmp0 (ip6_method, NM_SETTING_IP6_CONFIG_METHOD_AUTO) == 0; - ppp_cmd = create_pppd_cmd_line (manager, + ppp_cmd = create_pppd_cmd_line (self, s_ppp, pppoe_setting, adsl_setting, @@ -1109,8 +1109,8 @@ _ppp_manager_start (NMPPPManager *manager, _LOGI ("pppd started with pid %lld", (long long) priv->pid); - priv->ppp_watch_id = g_child_watch_add (priv->pid, (GChildWatchFunc) ppp_watch_cb, manager); - priv->ppp_timeout_handler = g_timeout_add_seconds (timeout_secs, pppd_timed_out, manager); + priv->ppp_watch_id = g_child_watch_add (priv->pid, (GChildWatchFunc) ppp_watch_cb, self); + priv->ppp_timeout_handler = g_timeout_add_seconds (timeout_secs, pppd_timed_out, self); priv->act_req = g_object_ref (req); out: @@ -1118,17 +1118,17 @@ out: nm_cmd_line_destroy (ppp_cmd); if (priv->pid <= 0) - nm_dbus_object_unexport (NM_DBUS_OBJECT (manager)); + nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); return priv->pid > 0; } static gboolean -_ppp_kill (NMPPPManager *manager, +_ppp_kill (NMPPPManager *self, NMUtilsKillChildAsyncCb callback, void *user_data) { - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); if (!priv->pid) { /* not PID. Signal that there was nothing to kill, which consequently @@ -1143,21 +1143,21 @@ _ppp_kill (NMPPPManager *manager, } static void -_ppp_cleanup (NMPPPManager *manager) +_ppp_cleanup (NMPPPManager *self) { NMPPPManagerPrivate *priv; - g_return_if_fail (NM_IS_PPP_MANAGER (manager)); + g_return_if_fail (NM_IS_PPP_MANAGER (self)); - priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + priv = NM_PPP_MANAGER_GET_PRIVATE (self); - cancel_get_secrets (manager); + cancel_get_secrets (self); nm_clear_g_source (&priv->monitor_id); if (priv->monitor_fd >= 0) { /* Get the stats one last time */ - monitor_cb (manager); + monitor_cb (self); nm_close (priv->monitor_fd); priv->monitor_fd = -1; } @@ -1169,7 +1169,7 @@ _ppp_cleanup (NMPPPManager *manager) /*****************************************************************************/ typedef struct { - NMPPPManager *manager; + NMPPPManager *self; GSimpleAsyncResult *result; GCancellable *cancellable; } StopContext; @@ -1181,7 +1181,7 @@ stop_context_complete (StopContext *ctx) g_object_unref (ctx->cancellable); g_simple_async_result_complete_in_idle (ctx->result); g_object_unref (ctx->result); - g_object_unref (ctx->manager); + g_object_unref (ctx->self); g_slice_free (StopContext, ctx); } @@ -1199,7 +1199,7 @@ stop_context_complete_if_cancelled (StopContext *ctx) } static gboolean -_ppp_manager_stop_finish (NMPPPManager *manager, +_ppp_manager_stop_finish (NMPPPManager *self, GAsyncResult *res, GError **error) { @@ -1220,19 +1220,19 @@ kill_child_ready (pid_t pid, } static void -_ppp_manager_stop_async (NMPPPManager *manager, +_ppp_manager_stop_async (NMPPPManager *self, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data) { - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); StopContext *ctx; - nm_dbus_object_unexport (NM_DBUS_OBJECT (manager)); + nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); ctx = g_slice_new0 (StopContext); - ctx->manager = g_object_ref (manager); - ctx->result = g_simple_async_result_new (G_OBJECT (manager), + ctx->self = g_object_ref (self); + ctx->result = g_simple_async_result_new (G_OBJECT (self), callback, user_data, _ppp_manager_stop_async); @@ -1243,7 +1243,7 @@ _ppp_manager_stop_async (NMPPPManager *manager, return; /* Cleanup internals */ - _ppp_cleanup (manager); + _ppp_cleanup (self); /* If no pppd running, we're done */ if (!priv->pid) { @@ -1251,20 +1251,20 @@ _ppp_manager_stop_async (NMPPPManager *manager, return; } - if (!_ppp_kill (manager, kill_child_ready, ctx)) + if (!_ppp_kill (self, kill_child_ready, ctx)) nm_assert_not_reached (); } static void -_ppp_manager_stop_sync (NMPPPManager *manager) +_ppp_manager_stop_sync (NMPPPManager *self) { - NMDBusObject *dbus = NM_DBUS_OBJECT (manager); + NMDBusObject *dbus = NM_DBUS_OBJECT (self); if (nm_dbus_object_is_exported (dbus)) nm_dbus_object_unexport (dbus); - _ppp_cleanup (manager); - _ppp_kill (manager, NULL, NULL); + _ppp_cleanup (self); + _ppp_kill (self, NULL, NULL); } /*****************************************************************************/ @@ -1305,9 +1305,9 @@ set_property (GObject *object, guint prop_id, /*****************************************************************************/ static void -nm_ppp_manager_init (NMPPPManager *manager) +nm_ppp_manager_init (NMPPPManager *self) { - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); + NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); priv->ifindex = -1; priv->monitor_fd = -1; From 43f67b42101d060c9e6b97486672a36f34bcec99 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 May 2018 12:14:46 +0200 Subject: [PATCH 4/5] ppp-manager: rework stopping NMPPPManager by merging async/sync methods Previously, there were two functions nm_ppp_manager_stop_sync() and nm_ppp_manager_stop_async(). However, stop-sync() would still kill the process asynchronously (with a 2 seconds timeout before sending SIGKILL). On the other hand, stop-async() did pretty much the same thing as sync-code, except also using the GAsyncResult. Merge the two functions. Stopping the instance for the most part can be done entirely synchrnous. The only thing that is asynchronous, is to wait for the process to terminate. For that, add a new callback argument to nm_ppp_manager_stop(). This replaces the GAsyncResult pattern. Also, always ensure that NetworkManager runs the mainloop at least as long until the process really terminated. Currently we don't get that right, and during shutdown we just stop iterating the mainloop. However, fix this from point of view of NMPPPManager and register a wait-object, that later will correctly delay shutdown. Also, NMDeviceWwan cared to wait (asynchronously) until pppd really terminated. Keep that functionality. nm_ppp_manager_stop() returns a handle that can be used to cancel the asynchrounous request and invoke the callback right away. However note, that even when cancelling the request, the wait-object that prevents shutdown of NetworkManager is kept around, so that we can be sure to properly clean up. --- src/devices/adsl/nm-device-adsl.c | 2 +- src/devices/nm-device-ethernet.c | 2 +- src/devices/nm-device-ppp.c | 2 +- src/devices/wwan/nm-modem.c | 61 +++++--- src/ppp/nm-ppp-manager-call.c | 34 ++--- src/ppp/nm-ppp-manager-call.h | 14 +- src/ppp/nm-ppp-manager.c | 230 ++++++++++++++++-------------- src/ppp/nm-ppp-manager.h | 7 + src/ppp/nm-ppp-plugin-api.h | 14 +- 9 files changed, 196 insertions(+), 170 deletions(-) diff --git a/src/devices/adsl/nm-device-adsl.c b/src/devices/adsl/nm-device-adsl.c index 5fcc823a20..1450a8369d 100644 --- a/src/devices/adsl/nm-device-adsl.c +++ b/src/devices/adsl/nm-device-adsl.c @@ -525,7 +525,7 @@ adsl_cleanup (NMDeviceAdsl *self) if (priv->ppp_manager) { g_signal_handlers_disconnect_by_func (priv->ppp_manager, G_CALLBACK (ppp_state_changed), self); g_signal_handlers_disconnect_by_func (priv->ppp_manager, G_CALLBACK (ppp_ip4_config), self); - nm_ppp_manager_stop_sync (priv->ppp_manager); + nm_ppp_manager_stop (priv->ppp_manager, NULL, NULL); g_clear_object (&priv->ppp_manager); } diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 43d137aab2..6c5e33e4cc 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -1343,7 +1343,7 @@ deactivate (NMDevice *device) nm_clear_g_source (&priv->pppoe_wait_id); if (priv->ppp_manager) { - nm_ppp_manager_stop_sync (priv->ppp_manager); + nm_ppp_manager_stop (priv->ppp_manager, NULL, NULL); g_clear_object (&priv->ppp_manager); } diff --git a/src/devices/nm-device-ppp.c b/src/devices/nm-device-ppp.c index 94df0caed2..a6abb228a6 100644 --- a/src/devices/nm-device-ppp.c +++ b/src/devices/nm-device-ppp.c @@ -239,7 +239,7 @@ deactivate (NMDevice *device) NMDevicePppPrivate *priv = NM_DEVICE_PPP_GET_PRIVATE (self); if (priv->ppp_manager) { - nm_ppp_manager_stop_sync (priv->ppp_manager); + nm_ppp_manager_stop (priv->ppp_manager, NULL, NULL); g_clear_object (&priv->ppp_manager); } } diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c index c49b1aac6a..fbe99cc30e 100644 --- a/src/devices/wwan/nm-modem.c +++ b/src/devices/wwan/nm-modem.c @@ -1131,7 +1131,7 @@ deactivate_cleanup (NMModem *self, NMDevice *device) if (priv->ppp_manager) { g_signal_handlers_disconnect_by_data (priv->ppp_manager, self); - nm_ppp_manager_stop_sync (priv->ppp_manager); + nm_ppp_manager_stop (priv->ppp_manager, NULL, NULL); g_clear_object (&priv->ppp_manager); } @@ -1177,11 +1177,19 @@ typedef struct { GSimpleAsyncResult *result; DeactivateContextStep step; NMPPPManager *ppp_manager; + NMPPPManagerStopHandle *ppp_stop_handle; + gulong ppp_stop_cancellable_id; } DeactivateContext; static void deactivate_context_complete (DeactivateContext *ctx) { + if (ctx->ppp_stop_handle) + nm_ppp_manager_stop_cancel (ctx->ppp_stop_handle); + + nm_assert (!ctx->ppp_stop_handle); + nm_assert (ctx->ppp_stop_cancellable_id == 0); + if (ctx->ppp_manager) g_object_unref (ctx->ppp_manager); if (ctx->cancellable) @@ -1223,25 +1231,36 @@ disconnect_ready (NMModem *self, static void ppp_manager_stop_ready (NMPPPManager *ppp_manager, - GAsyncResult *res, - DeactivateContext *ctx) + NMPPPManagerStopHandle *handle, + gboolean was_cancelled, + gpointer user_data) { - NMModem *self = ctx->self; - GError *error = NULL; + DeactivateContext *ctx = user_data; - if (!nm_ppp_manager_stop_finish (ppp_manager, res, &error)) { - _LOGW ("cannot stop PPP manager: %s", - error->message); - g_simple_async_result_take_error (ctx->result, error); - deactivate_context_complete (ctx); - return; + nm_assert (ctx->ppp_stop_handle == handle); + ctx->ppp_stop_handle = NULL; + + if (ctx->ppp_stop_cancellable_id) { + g_cancellable_disconnect (ctx->cancellable, + nm_steal_int (&ctx->ppp_stop_cancellable_id)); } - /* Go on */ + if (was_cancelled) + return; + ctx->step++; deactivate_step (ctx); } +static void +ppp_manager_stop_cancelled (GCancellable *cancellable, + gpointer user_data) +{ + DeactivateContext *ctx = user_data; + + nm_ppp_manager_stop_cancel (ctx->ppp_stop_handle); +} + static void deactivate_step (DeactivateContext *ctx) { @@ -1271,10 +1290,16 @@ deactivate_step (DeactivateContext *ctx) case DEACTIVATE_CONTEXT_STEP_PPP_MANAGER_STOP: /* If we have a PPP manager, stop it */ if (ctx->ppp_manager) { - nm_ppp_manager_stop_async (ctx->ppp_manager, - ctx->cancellable, - (GAsyncReadyCallback) ppp_manager_stop_ready, - ctx); + nm_assert (!ctx->ppp_stop_handle); + if (ctx->cancellable) { + ctx->ppp_stop_cancellable_id = g_cancellable_connect (ctx->cancellable, + G_CALLBACK (ppp_manager_stop_cancelled), + ctx, + NULL); + } + ctx->ppp_stop_handle = nm_ppp_manager_stop (ctx->ppp_manager, + ppp_manager_stop_ready, + ctx); return; } ctx->step++; @@ -1313,7 +1338,9 @@ nm_modem_deactivate_async (NMModem *self, callback, user_data, nm_modem_deactivate_async); - ctx->cancellable = cancellable ? g_object_ref (cancellable) : NULL; + /* FIXME(shutdown): we always require a cancellable, otherwise we cannot + * do a coordinated shutdown. */ + ctx->cancellable = nm_g_object_ref (cancellable); /* Start */ ctx->step = DEACTIVATE_CONTEXT_STEP_FIRST; diff --git a/src/ppp/nm-ppp-manager-call.c b/src/ppp/nm-ppp-manager-call.c index ed70f68790..3d6fee49a0 100644 --- a/src/ppp/nm-ppp-manager-call.c +++ b/src/ppp/nm-ppp-manager-call.c @@ -82,9 +82,8 @@ nm_ppp_manager_create (const char *iface, GError **error) nm_assert (ops); nm_assert (ops->create); nm_assert (ops->start); - nm_assert (ops->stop_async); - nm_assert (ops->stop_finish); - nm_assert (ops->stop_sync); + nm_assert (ops->stop); + nm_assert (ops->stop_cancel); ppp_ops = ops; @@ -125,32 +124,21 @@ nm_ppp_manager_start (NMPPPManager *self, return ppp_ops->start (self, req, ppp_name, timeout_secs, baud_override, err); } -void -nm_ppp_manager_stop_async (NMPPPManager *self, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data) +NMPPPManagerStopHandle * +nm_ppp_manager_stop (NMPPPManager *self, + NMPPPManagerStopCallback callback, + gpointer user_data) { - g_return_if_fail (ppp_ops); + g_return_val_if_fail (ppp_ops, NULL); - ppp_ops->stop_async (self, cancellable, callback, user_data); -} - -gboolean -nm_ppp_manager_stop_finish (NMPPPManager *self, - GAsyncResult *res, - GError **error) -{ - g_return_val_if_fail (ppp_ops, FALSE); - - return ppp_ops->stop_finish (self, res, error); + return ppp_ops->stop (self, callback, user_data); } void -nm_ppp_manager_stop_sync (NMPPPManager *self) +nm_ppp_manager_stop_cancel (NMPPPManagerStopHandle *handle) { g_return_if_fail (ppp_ops); + g_return_if_fail (handle); - ppp_ops->stop_sync (self); + ppp_ops->stop_cancel (handle); } - diff --git a/src/ppp/nm-ppp-manager-call.h b/src/ppp/nm-ppp-manager-call.h index 2258ae08f7..daf8a82e68 100644 --- a/src/ppp/nm-ppp-manager-call.h +++ b/src/ppp/nm-ppp-manager-call.h @@ -38,13 +38,11 @@ gboolean nm_ppp_manager_start (NMPPPManager *self, guint32 timeout_secs, guint baud_override, GError **error); -void nm_ppp_manager_stop_async (NMPPPManager *self, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data); -gboolean nm_ppp_manager_stop_finish (NMPPPManager *self, - GAsyncResult *res, - GError **error); -void nm_ppp_manager_stop_sync (NMPPPManager *self); + +NMPPPManagerStopHandle *nm_ppp_manager_stop (NMPPPManager *self, + NMPPPManagerStopCallback callback, + gpointer user_data); + +void nm_ppp_manager_stop_cancel (NMPPPManagerStopHandle *handle); #endif /* __NM_PPP_MANAGER_CALL_H__ */ diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c index a91e5cb3c5..e7de5d4a2e 100644 --- a/src/ppp/nm-ppp-manager.c +++ b/src/ppp/nm-ppp-manager.c @@ -135,9 +135,10 @@ G_DEFINE_TYPE (NMPPPManager, nm_ppp_manager, NM_TYPE_DBUS_OBJECT) /*****************************************************************************/ static void _ppp_cleanup (NMPPPManager *self); -static gboolean _ppp_kill (NMPPPManager *self, - NMUtilsKillChildAsyncCb callback, - void *user_data); + +static NMPPPManagerStopHandle *_ppp_manager_stop (NMPPPManager *self, + NMPPPManagerStopCallback callback, + gpointer user_data); /*****************************************************************************/ @@ -782,8 +783,7 @@ pppd_timed_out (gpointer data) NMPPPManager *self = NM_PPP_MANAGER (data); _LOGW ("pppd timed out or didn't initialize our dbus module"); - _ppp_cleanup (self); - _ppp_kill (self, NULL, NULL); + _ppp_manager_stop (self, NULL, NULL); g_signal_emit (self, signals[STATE_CHANGED], 0, (guint) NM_PPP_STATUS_DEAD); @@ -1123,25 +1123,6 @@ out: return priv->pid > 0; } -static gboolean -_ppp_kill (NMPPPManager *self, - NMUtilsKillChildAsyncCb callback, - void *user_data) -{ - NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); - - if (!priv->pid) { - /* not PID. Signal that there was nothing to kill, which consequently - * implies that the callback will not be invoked. */ - return FALSE; - } - - nm_utils_kill_child_async (nm_steal_int (&priv->pid), - SIGTERM, LOGD_PPP, "pppd", 2000, - callback, user_data); - return TRUE; -} - static void _ppp_cleanup (NMPPPManager *self) { @@ -1168,103 +1149,132 @@ _ppp_cleanup (NMPPPManager *self) /*****************************************************************************/ -typedef struct { +struct _NMPPPManagerStopHandle { NMPPPManager *self; - GSimpleAsyncResult *result; - GCancellable *cancellable; -} StopContext; + NMPPPManagerStopCallback callback; + gpointer user_data; + + /* this object delays shutdown, because we still need to wait until + * pppd process terminated. */ + GObject *shutdown_waitobj; + + guint idle_id; +}; static void -stop_context_complete (StopContext *ctx) +_stop_handle_complete (NMPPPManagerStopHandle *handle, gboolean was_cancelled) { - if (ctx->cancellable) - g_object_unref (ctx->cancellable); - g_simple_async_result_complete_in_idle (ctx->result); - g_object_unref (ctx->result); - g_object_unref (ctx->self); - g_slice_free (StopContext, ctx); -} + gs_unref_object NMPPPManager *self = NULL; + NMPPPManagerStopCallback callback; -static gboolean -stop_context_complete_if_cancelled (StopContext *ctx) -{ - GError *error = NULL; + self = g_steal_pointer (&handle->self); + if (!self) + return; - if (g_cancellable_set_error_if_cancelled (ctx->cancellable, &error)) { - g_simple_async_result_take_error (ctx->result, error); - stop_context_complete (ctx); - return TRUE; - } - return FALSE; -} + if (!handle->callback) + return; -static gboolean -_ppp_manager_stop_finish (NMPPPManager *self, - GAsyncResult *res, - GError **error) -{ - return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error); + callback = handle->callback; + handle->callback = NULL; + callback (self, handle, was_cancelled, handle->user_data); } static void -kill_child_ready (pid_t pid, - gboolean success, - int child_status, +_stop_handle_destroy (NMPPPManagerStopHandle *handle, gboolean was_cancelled) +{ + _stop_handle_complete (handle, was_cancelled); + nm_clear_g_source (&handle->idle_id); + g_clear_object (&handle->shutdown_waitobj); + g_slice_free (NMPPPManagerStopHandle, handle); +} + +static void +_stop_child_cb (pid_t pid, + gboolean success, + int child_status, + gpointer user_data) +{ + _stop_handle_destroy (user_data, FALSE); +} + +static gboolean +_stop_idle_cb (gpointer user_data) +{ + NMPPPManagerStopHandle *handle = user_data; + + handle->idle_id = 0; + _stop_handle_destroy (handle, FALSE); + return G_SOURCE_REMOVE; +} + +static NMPPPManagerStopHandle * +_ppp_manager_stop (NMPPPManager *self, + NMPPPManagerStopCallback callback, gpointer user_data) -{ - StopContext *ctx = user_data; - - if (stop_context_complete_if_cancelled (ctx)) - return; - stop_context_complete (ctx); -} - -static void -_ppp_manager_stop_async (NMPPPManager *self, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data) { NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); - StopContext *ctx; - - nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); - - ctx = g_slice_new0 (StopContext); - ctx->self = g_object_ref (self); - ctx->result = g_simple_async_result_new (G_OBJECT (self), - callback, - user_data, - _ppp_manager_stop_async); - - /* Setup cancellable */ - ctx->cancellable = cancellable ? g_object_ref (cancellable) : NULL; - if (stop_context_complete_if_cancelled (ctx)) - return; - - /* Cleanup internals */ - _ppp_cleanup (self); - - /* If no pppd running, we're done */ - if (!priv->pid) { - stop_context_complete (ctx); - return; - } - - if (!_ppp_kill (self, kill_child_ready, ctx)) - nm_assert_not_reached (); -} - -static void -_ppp_manager_stop_sync (NMPPPManager *self) -{ NMDBusObject *dbus = NM_DBUS_OBJECT (self); + NMPPPManagerStopHandle *handle; if (nm_dbus_object_is_exported (dbus)) nm_dbus_object_unexport (dbus); _ppp_cleanup (self); - _ppp_kill (self, NULL, NULL); + + if ( !priv->pid + && !callback) { + /* nothing to do further... + * + * In this case, we return a %NULL handle. The caller cannot cancel this + * event, but clearly he is not waiting for a callback anyway. */ + return NULL; + } + + handle = g_slice_new0 (NMPPPManagerStopHandle); + handle->self = g_object_ref (self); + handle->callback = callback; + handle->user_data = user_data; + + if (!priv->pid) { + /* No PID. There is nothing to kill, however, invoke the callback in + * an idle handler. + * + * Note that we don't register nm_shutdown_wait_obj_register(). + * In order for shutdown to work properly, the caller must always + * explicitly cancel the action to go down. With the idle-handler, + * cancelling the handle completes the request. */ + handle->idle_id = g_idle_add (_stop_idle_cb, handle); + return handle; + } + + /* we really want to kill the process and delay shutdown of NetworkManager + * until the process terminated. We do that, by registering an object + * that delays shutdown. */ + handle->shutdown_waitobj = g_object_new (G_TYPE_OBJECT, NULL); + nm_shutdown_wait_obj_register (handle->shutdown_waitobj, "ppp-manager-wait-kill-pppd"); + nm_utils_kill_child_async (nm_steal_int (&priv->pid), + SIGTERM, LOGD_PPP, "pppd", 2000, + _stop_child_cb, handle); + + return handle; +} + +static void +_ppp_manager_stop_cancel (NMPPPManagerStopHandle *handle) +{ + g_return_if_fail (handle); + g_return_if_fail (NM_IS_PPP_MANAGER (handle->self)); + + if (handle->idle_id) { + /* we can complete this fake handle right away. */ + _stop_handle_destroy (handle, TRUE); + return; + } + + /* a real handle. Only invoke the callback (synchronously). This marks + * the handle as handled, but it keeps shutdown_waitobj around, until + * nm_utils_kill_child_async() returns. */ + _stop_handle_complete (handle, TRUE); } /*****************************************************************************/ @@ -1331,14 +1341,13 @@ static void dispose (GObject *object) { NMPPPManager *self = (NMPPPManager *) object; - NMDBusObject *dbus = NM_DBUS_OBJECT (self); NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); - if (nm_dbus_object_is_exported (dbus)) - nm_dbus_object_unexport (dbus); - - _ppp_cleanup (self); - _ppp_kill (self, NULL, NULL); + /* we expect the user to first stop the manager. As fallback, + * still stop. */ + g_warn_if_fail (!priv->pid); + g_warn_if_fail (!nm_dbus_object_is_exported (NM_DBUS_OBJECT (self))); + _ppp_manager_stop (self, NULL, NULL); g_clear_object (&priv->act_req); @@ -1484,7 +1493,6 @@ NMPPPOps ppp_ops = { .create = _ppp_manager_new, .set_route_parameters = _ppp_manager_set_route_parameters, .start = _ppp_manager_start, - .stop_async = _ppp_manager_stop_async, - .stop_finish = _ppp_manager_stop_finish, - .stop_sync = _ppp_manager_stop_sync, + .stop = _ppp_manager_stop, + .stop_cancel = _ppp_manager_stop_cancel, }; diff --git a/src/ppp/nm-ppp-manager.h b/src/ppp/nm-ppp-manager.h index 5457726e96..ec0ca46e71 100644 --- a/src/ppp/nm-ppp-manager.h +++ b/src/ppp/nm-ppp-manager.h @@ -32,4 +32,11 @@ typedef struct _NMPPPManager NMPPPManager; +typedef struct _NMPPPManagerStopHandle NMPPPManagerStopHandle; + +typedef void (*NMPPPManagerStopCallback) (NMPPPManager *manager, + NMPPPManagerStopHandle *handle, + gboolean was_cancelled, + gpointer user_data); + #endif /* __NM_PPP_MANAGER_H__ */ diff --git a/src/ppp/nm-ppp-plugin-api.h b/src/ppp/nm-ppp-plugin-api.h index bb53690c63..558de2c2c2 100644 --- a/src/ppp/nm-ppp-plugin-api.h +++ b/src/ppp/nm-ppp-plugin-api.h @@ -21,6 +21,8 @@ #ifndef __NM_PPP_PLUGIN_API_H__ #define __NM_PPP_PLUGIN_API_H__ +#include "nm-ppp-manager.h" + typedef const struct { NMPPPManager *(*create) (const char *iface); @@ -37,16 +39,12 @@ typedef const struct { guint baud_override, GError **err); - void (*stop_async) (NMPPPManager *manager, - GCancellable *cancellable, - GAsyncReadyCallback callback, - gpointer user_data); + NMPPPManagerStopHandle *(*stop) (NMPPPManager *manager, + NMPPPManagerStopCallback callback, + gpointer user_data); - gboolean (*stop_finish) (NMPPPManager *manager, - GAsyncResult *res, - GError **error); + void (*stop_cancel) (NMPPPManagerStopHandle *handle); - void (*stop_sync) (NMPPPManager *manager); } NMPPPOps; #endif /* __NM_PPP_PLUGIN_API_H__ */ From eaf36db68bd4bdc1b626d672079dd241b4eea53e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 18 May 2018 14:37:14 +0200 Subject: [PATCH 5/5] core: add and use NM_SHUTDOWN_TIMEOUT_MS as duration that we plan for shutdown nm_ppp_manager_stop() wants to ensure that the pppd process is really gone. For that it uses nm_utils_kill_child_async() to first send SIGTERM, and sending SIGKILL after a timeout. Later, we want to fix shutdown of NetworkManager to iterate the mainloop during shutdown, so that such operations are still handled. However, we can only delay shutdown for a certain time. After a timeout (NM_SHUTDOWN_TIMEOUT_MS plus NM_SHUTDOWN_TIMEOUT_MS_GRACE) we really have to give up and terminate. That means, the right amount of time between sending SIGTERM and SIGKILL is exactly NM_SHUTDOWN_TIMEOUT_MS. Hopefully that is of course sufficient in the first place. If not, send SIGKILL afterwards, and give a bit more time (NM_SHUTDOWN_TIMEOUT_MS_GRACE) to reap the child. And if all this time is still not enough, something is really odd and we abort waiting, with a warning in the logfile. Since we don't properly handle shutdown yet, the description above is not really true. But with this patch, we fix it from point of view of NMPPPManager. --- src/NetworkManagerUtils.h | 19 +++++++++++++++++++ src/ppp/nm-ppp-manager.c | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index b8d3a4f064..b26d08bdce 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -56,6 +56,25 @@ int nm_match_spec_device_by_pllink (const NMPlatformLink *pllink, /*****************************************************************************/ +/* during shutdown, there are two relevant timeouts. One is + * NM_SHUTDOWN_TIMEOUT_MS which is plenty of time, that we give for all + * actions to complete. Of course, during shutdown components should hurry + * to cleanup. + * + * When we initiate shutdown, we should start killing child processes + * with SIGTERM. If they don't complete within NM_SHUTDOWN_TIMEOUT_MS, we send + * SIGKILL. + * + * After NM_SHUTDOWN_TIMEOUT_MS, NetworkManager will however not yet terminate right + * away. It iterates the mainloop for another NM_SHUTDOWN_TIMEOUT_MS_EXTRA. This + * should give time to reap the child process (after SIGKILL). + * + * So, the maxiumum time we should wait before sending SIGKILL should be at most + * NM_SHUTDOWN_TIMEOUT_MS. + */ +#define NM_SHUTDOWN_TIMEOUT_MS 1500 +#define NM_SHUTDOWN_TIMEOUT_MS_WATCHDOG 500 + typedef struct _NMShutdownWaitObjHandle NMShutdownWaitObjHandle; NMShutdownWaitObjHandle *_nm_shutdown_wait_obj_register (GObject *watched_obj, diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c index e7de5d4a2e..fc658bec93 100644 --- a/src/ppp/nm-ppp-manager.c +++ b/src/ppp/nm-ppp-manager.c @@ -1253,7 +1253,8 @@ _ppp_manager_stop (NMPPPManager *self, handle->shutdown_waitobj = g_object_new (G_TYPE_OBJECT, NULL); nm_shutdown_wait_obj_register (handle->shutdown_waitobj, "ppp-manager-wait-kill-pppd"); nm_utils_kill_child_async (nm_steal_int (&priv->pid), - SIGTERM, LOGD_PPP, "pppd", 2000, + SIGTERM, LOGD_PPP, "pppd", + NM_SHUTDOWN_TIMEOUT_MS, _stop_child_cb, handle); return handle;