From cf79615169b4c2df8c5d99aa43e370e90e2d82ef Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 15 Feb 2018 17:58:28 +0100 Subject: [PATCH] ovs: add error code for callbacks to indicate NM is quitting When NM quits it destroys all singletons including NMOvsdb, which invokes callbacks for every pending method call. In the shutdown, extra care must be taken to not access objects that are already in a inconsistent state; for example here, the callback changes the device state, and this causes an access to data that has already been cleared: #0 _g_log_abort (breakpoint=breakpoint@entry=1) at gmessages.c:554 #1 g_logv (log_domain=0x5635653b6817 "NetworkManager", log_level=G_LOG_LEVEL_CRITICAL, format=, args=args@entry=0x7fffb4b2c1e0) at gmessages.c:1362 #2 g_log (log_domain=log_domain@entry=0x5635653b6817 "NetworkManager", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7fbb3f58fa4a "%s: assertion '%s' failed") at gmessages.c:1403 #3 g_return_if_fail_warning (log_domain=log_domain@entry=0x5635653b6817 "NetworkManager", pretty_function=pretty_function@entry=0x5635653b6b00 <__func__.34463> "nm_device_factory_manager_find_factory_for_connection", expression=expression@entry=0x5635653b6719 "factories_by_setting") at gmessages.c:2702 #4 nm_device_factory_manager_find_factory_for_connection (connection=connection@entry=0x56356627e0e0) at src/devices/nm-device-factory.c:243 #5 nm_manager_get_connection_iface (self=0x563566241080 [NMManager], connection=connection@entry=0x56356627e0e0, out_parent=out_parent@entry=0x0, error=error@entry=0x0) at src/nm-manager.c:1458 #6 check_connection_compatible (self=, connection=0x56356627e0e0) at src/devices/nm-device.c:4679 #7 check_connection_compatible (device=0x56356647b1b0 [NMDeviceOvsInterface], connection=0x56356627e0e0) at src/devices/ovs/nm-device-ovs-interface.c:95 #8 _nm_device_check_connection_available (self=0x56356647b1b0 [NMDeviceOvsInterface], connection=0x56356627e0e0, flags=NM_DEVICE_CHECK_CON_AVAILABLE_NONE, specific_object=0x0) at src/devices/nm-device.c:12102 #9 nm_device_check_connection_available (self=self@entry=0x56356647b1b0 [NMDeviceOvsInterface], connection=0x56356627e0e0, flags=flags@entry=NM_DEVICE_CHECK_CON_AVAILABLE_NONE, specific_object=specific_object@entry=0x0) at src/devices/nm-device.c:12131 #10 nm_device_recheck_available_connections (self=self@entry=0x56356647b1b0 [NMDeviceOvsInterface]) at src/devices/nm-device.c:12238 #11 _set_state_full (self=self@entry=0x56356647b1b0 [NMDeviceOvsInterface], state=state@entry=NM_DEVICE_STATE_FAILED, reason=reason@entry=NM_DEVICE_STATE_REASON_OVSDB_FAILED, quitting=quitting@entry=0) at src/devices/nm-device.c:13065 #12 nm_device_state_changed (self=self@entry=0x56356647b1b0 [NMDeviceOvsInterface], state=state@entry=NM_DEVICE_STATE_FAILED, reason=reason@entry=NM_DEVICE_STATE_REASON_OVSDB_FAILED) at src/devices/nm-device.c:13328 #13 del_iface_cb (error=, user_data=0x56356647b1b0) at src/devices/ovs/nm-device-ovs-port.c:160 #14 _transact_cb (self=self@entry=0x5635662b9ba0 [NMOvsdb], result=result@entry=0x0, error=0x563566259a10, user_data=user_data@entry=0x5635662ff320) at src/devices/ovs/nm-ovsdb.c:1449 #15 ovsdb_disconnect (self=self@entry=0x5635662b9ba0 [NMOvsdb]) at src/devices/ovs/nm-ovsdb.c:1331 #16 dispose (object=0x5635662b9ba0 [NMOvsdb]) at src/devices/ovs/nm-ovsdb.c:1558 #17 g_object_unref (_object=0x5635662b9ba0) at gobject.c:3293 #18 _nm_singleton_instance_destroy () at src/nm-core-utils.c:138 #19 _dl_fini () at dl-fini.c:253 #20 __run_exit_handlers (status=status@entry=0, listp=0x7fbb3e1ad6c8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:77 #21 __GI_exit (status=status@entry=0) at exit.c:99 #22 main (argc=1, argv=0x7fffb4b2cc38) at src/main.c:468 Add a new error code to indicate to callbacks that we are quitting and no further action must be taken. This is preferable to having additional references because it allows us to free the resources owned by callbacks immediately, while references can easily create loops. https://bugzilla.redhat.com/show_bug.cgi?id=1543871 --- src/devices/ovs/nm-device-ovs-port.c | 6 ++++-- src/devices/ovs/nm-ovsdb.c | 26 +++++++++++++------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/devices/ovs/nm-device-ovs-port.c b/src/devices/ovs/nm-device-ovs-port.c index 319abfb1be..cb0915afe6 100644 --- a/src/devices/ovs/nm-device-ovs-port.c +++ b/src/devices/ovs/nm-device-ovs-port.c @@ -114,7 +114,8 @@ add_iface_cb (GError *error, gpointer user_data) { NMDevice *slave = user_data; - if (error) { + if ( error + && !g_error_matches (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING)) { nm_log_warn (LOGD_DEVICE, "device %s could not be added to a ovs port: %s", nm_device_get_iface (slave), error->message); nm_device_state_changed (slave, @@ -153,7 +154,8 @@ del_iface_cb (GError *error, gpointer user_data) { NMDevice *slave = user_data; - if (error) { + if ( error + && !g_error_matches (error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING)) { nm_log_warn (LOGD_DEVICE, "device %s could not be removed from a ovs port: %s", nm_device_get_iface (slave), error->message); nm_device_state_changed (slave, diff --git a/src/devices/ovs/nm-ovsdb.c b/src/devices/ovs/nm-ovsdb.c index 6d28bd2c55..08a8d2c1da 100644 --- a/src/devices/ovs/nm-ovsdb.c +++ b/src/devices/ovs/nm-ovsdb.c @@ -102,7 +102,7 @@ NM_DEFINE_SINGLETON_GETTER (NMOvsdb, nm_ovsdb_get, NM_TYPE_OVSDB); /*****************************************************************************/ static void ovsdb_try_connect (NMOvsdb *self); -static void ovsdb_disconnect (NMOvsdb *self); +static void ovsdb_disconnect (NMOvsdb *self, gboolean is_disposing); static void ovsdb_read (NMOvsdb *self); static void ovsdb_write (NMOvsdb *self); static void ovsdb_next_command (NMOvsdb *self); @@ -1086,7 +1086,7 @@ ovsdb_got_msg (NMOvsdb *self, json_t *msg) "result", &result, "error", &error) == -1) { _LOGW ("couldn't grok the message: %s", json_error.text); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } @@ -1097,7 +1097,7 @@ ovsdb_got_msg (NMOvsdb *self, json_t *msg) /* It's a method call! */ if (!params) { _LOGW ("a method call with no params: '%s'", method); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } @@ -1117,13 +1117,13 @@ ovsdb_got_msg (NMOvsdb *self, json_t *msg) /* This is a response to a method call. */ if (!priv->calls->len) { _LOGE ("there are no queued calls expecting response %" G_GUINT64_FORMAT, id); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } call = &g_array_index (priv->calls, OvsdbMethodCall, 0); if (call->id != id) { _LOGE ("expected a response to call %" G_GUINT64_FORMAT ", not %" G_GUINT64_FORMAT, call->id, id); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } /* Cool, we found a corresponsing call. Finish it. */ @@ -1202,7 +1202,7 @@ ovsdb_read_cb (GObject *source_object, GAsyncResult *res, gpointer user_data) if (size == -1) { _LOGW ("short read from ovsdb: %s", error->message); g_clear_error (&error); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } @@ -1250,7 +1250,7 @@ ovsdb_write_cb (GObject *source_object, GAsyncResult *res, gpointer user_data) if (size == -1) { _LOGW ("short write to ovsdb: %s", error->message); g_clear_error (&error); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); return; } @@ -1293,7 +1293,7 @@ ovsdb_write (NMOvsdb *self) * puts us back in sync. */ static void -ovsdb_disconnect (NMOvsdb *self) +ovsdb_disconnect (NMOvsdb *self, gboolean is_disposing) { NMOvsdbPrivate *priv = NM_OVSDB_GET_PRIVATE (self); OvsdbMethodCall *call; @@ -1306,7 +1306,7 @@ ovsdb_disconnect (NMOvsdb *self) while (priv->calls->len) { error = NULL; call = &g_array_index (priv->calls, OvsdbMethodCall, priv->calls->len - 1); - g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED, "Cancelled"); + nm_utils_error_set_cancelled (&error, is_disposing, "NMOvsdb"); callback = call->callback; user_data = call->user_data; @@ -1326,9 +1326,9 @@ static void _monitor_bridges_cb (NMOvsdb *self, json_t *result, GError *error, gpointer user_data) { if (error) { - if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + if (!nm_utils_error_is_cancelled (error, TRUE)) { _LOGI ("%s", error->message); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); } g_clear_error (&error); @@ -1354,7 +1354,7 @@ _client_connect_cb (GObject *source_object, GAsyncResult *res, gpointer user_dat if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) _LOGI ("%s", error->message); - ovsdb_disconnect (self); + ovsdb_disconnect (self, FALSE); g_clear_error (&error); return; } @@ -1538,7 +1538,7 @@ dispose (GObject *object) NMOvsdb *self = NM_OVSDB (object); NMOvsdbPrivate *priv = NM_OVSDB_GET_PRIVATE (self); - ovsdb_disconnect (self); + ovsdb_disconnect (self, TRUE); g_string_free (priv->input, TRUE); priv->input = NULL;