merge: branch 'lr/object-removal'

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1276

4 files changed, 141 insertions, 93 deletions

diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c
index 0e7d957c80..7935b4bb81 100644
--- a/src/libnm-client-impl/nm-client.c
+++ b/src/libnm-client-impl/nm-client.c
@@ -2630,7 +2630,16 @@ _obj_handle_dbus_iface_changes(NMClient            *self,
 
     if (is_removed) {
         for (i_prop = 0; i_prop < db_iface_data->dbus_iface.meta->n_dbus_properties; i_prop++) {
-            _obj_handle_dbus_prop_changes(self, dbobj, db_iface_data, i_prop, NULL);
+            const GVariantType *dbus_type =
+                db_iface_data->dbus_iface.meta->dbus_properties[i_prop].dbus_type;
+
+            /* Unset properties that can potentially contain objects, to release them,
+             * but keep the rest around, because it might still make sense to know what
+             * they were (e.g. when a device has been removed we'd like know what interface
+             * name it had, or keep the state to avoid spurious state change into UNKNOWN). */
+            if (g_variant_type_is_array(dbus_type)
+                || g_variant_type_equal(dbus_type, G_VARIANT_TYPE_OBJECT_PATH))
+                _obj_handle_dbus_prop_changes(self, dbobj, db_iface_data, i_prop, NULL);
         }
     } else {
         while ((db_prop_data = c_list_first_entry(&db_iface_data->changed_prop_lst_head,
diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c
index 07b0789b55..5de7c6a5b6 100644
--- a/src/libnm-client-impl/tests/test-nm-client.c
+++ b/src/libnm-client-impl/tests/test-nm-client.c
@@ -498,7 +498,8 @@ nm_running_changed(GObject *client, GParamSpec *pspec, gpointer user_data)
 {
     int *running_changed = user_data;
 
-    (*running_changed)++;
+    if (running_changed)
+        (*running_changed)++;
     g_main_loop_quit(gl.loop);
 }
 
@@ -791,50 +792,15 @@ activate_cb(GObject *object, GAsyncResult *result, gpointer user_data)
         g_main_loop_quit(info->loop);
 }
 
-static void
-_dev_eth0_1_state_changed_cb(NMDevice           *device,
-                             NMDeviceState       new_state,
-                             NMDeviceState       old_state,
-                             NMDeviceStateReason reason,
-                             int                *p_count_call)
+static NMClient *
+_activate_virtual(NMTstcServiceInfo *sinfo)
 {
-    const GPtrArray *arr;
-
-    g_assert(p_count_call);
-    g_assert_cmpint(*p_count_call, ==, 0);
-
-    (*p_count_call)++;
-
-    g_assert(NM_IS_DEVICE_VLAN(device));
-
-    g_assert_cmpint(old_state, >=, NM_DEVICE_STATE_PREPARE);
-    g_assert_cmpint(old_state, <=, NM_DEVICE_STATE_ACTIVATED);
-    g_assert_cmpint(new_state, ==, NM_DEVICE_STATE_UNKNOWN);
-
-    arr = nm_device_get_available_connections(device);
-    g_assert(arr);
-    g_assert_cmpint(arr->len, ==, 0);
-
-    g_assert(!nm_device_get_active_connection(device));
-}
-
-static void
-test_activate_virtual(void)
-{
-    nmtstc_auto_service_cleanup NMTstcServiceInfo *sinfo  = NULL;
-    gs_unref_object NMClient                      *client = NULL;
-    NMConnection                                  *conn;
-    NMSettingConnection                           *s_con;
-    NMSettingVlan                                 *s_vlan;
-    TestACInfo                                     info      = {gl.loop, NULL, 0};
-    TestConnectionInfo                             conn_info = {gl.loop, NULL};
-
-    if (nmtst_test_skip_slow())
-        return;
-
-    sinfo = nmtstc_service_init();
-    if (!nmtstc_service_available(sinfo))
-        return;
+    NMClient            *client = NULL;
+    NMConnection        *conn;
+    NMSettingConnection *s_con;
+    NMSettingVlan       *s_vlan;
+    TestACInfo           info      = {gl.loop, NULL, 0};
+    TestConnectionInfo   conn_info = {gl.loop, NULL};
 
     client = nmtstc_client_new(TRUE);
 
@@ -881,55 +847,97 @@ test_activate_virtual(void)
         nm_clear_g_signal_handler(info.device, &info.ac_signal_id);
     }
 
-    if (nmtst_get_rand_bool()) {
-        /* OK, enough for this run. Let's see whether we can tear down
-         * successfully at this point. */
+    return client;
+}
+
+static void
+test_activate_virtual(void)
+{
+    nmtstc_auto_service_cleanup NMTstcServiceInfo *sinfo = NULL;
+
+    if (nmtst_test_skip_slow())
         return;
-    }
 
-    {
-        NMDevice           *dev_eth0_1;
-        NMActiveConnection *ac;
-        const GPtrArray    *arr;
-        gulong              sig_id;
-        int                 call_count = 0;
-        gboolean            take_ref   = nmtst_get_rand_bool();
+    sinfo = nmtstc_service_init();
+    if (!nmtstc_service_available(sinfo))
+        return;
 
-        /* ensure we got all the necessary events in place. */
-        nmtst_main_loop_run(gl.loop, 50);
+    g_object_unref(_activate_virtual(sinfo));
+}
 
-        dev_eth0_1 = nm_client_get_device_by_iface(client, "eth0.1");
-        g_assert(NM_IS_DEVICE_VLAN(dev_eth0_1));
-        if (take_ref)
-            g_object_ref(dev_eth0_1);
+static void
+_client_dev_removed(NMClient *client, NMDevice *device, int *p_count_call)
+{
+    const GPtrArray *arr;
 
-        arr = nm_device_get_available_connections(dev_eth0_1);
-        g_assert(arr);
-        g_assert_cmpint(arr->len, ==, 1);
+    (*p_count_call)++;
 
-        ac = nm_device_get_active_connection(dev_eth0_1);
-        g_assert(NM_IS_ACTIVE_CONNECTION(ac));
+    arr = nm_device_get_available_connections(device);
+    g_assert(arr);
+    g_assert_cmpint(arr->len, ==, 0);
 
-        sig_id = g_signal_connect(dev_eth0_1,
-                                  "state-changed",
-                                  G_CALLBACK(_dev_eth0_1_state_changed_cb),
-                                  &call_count);
+    g_assert(!nm_device_get_active_connection(device));
+}
 
-        g_clear_object(&client);
+static void
+test_activate_virtual_teardown(gconstpointer user_data)
+{
+    nmtstc_auto_service_cleanup NMTstcServiceInfo *sinfo  = NULL;
+    gs_unref_object NMClient                      *client = NULL;
+    NMDevice                                      *dev_eth0_1;
+    NMActiveConnection                            *ac;
+    const GPtrArray                               *arr;
+    int                                            call_count       = 0;
+    gboolean                                       take_ref         = nmtst_get_rand_bool();
+    gboolean                                       teardown_service = GPOINTER_TO_INT(user_data);
 
-        g_assert_cmpint(call_count, ==, 1);
+    if (nmtst_test_skip_slow())
+        return;
+
+    sinfo = nmtstc_service_init();
+    if (!nmtstc_service_available(sinfo))
+        return;
 
-        if (take_ref) {
-            arr = nm_device_get_available_connections(dev_eth0_1);
-            g_assert(arr);
-            g_assert_cmpint(arr->len, ==, 0);
+    client = _activate_virtual(sinfo);
 
-            g_assert(!nm_device_get_active_connection(dev_eth0_1));
+    /* ensure we got all the necessary events in place. */
+    nmtst_main_loop_run(gl.loop, 50);
 
-            nm_clear_g_signal_handler(dev_eth0_1, &sig_id);
+    dev_eth0_1 = nm_client_get_device_by_iface(client, "eth0.1");
+    g_assert(NM_IS_DEVICE_VLAN(dev_eth0_1));
+    if (take_ref)
+        g_object_ref(dev_eth0_1);
 
-            g_object_unref(dev_eth0_1);
-        }
+    arr = nm_device_get_available_connections(dev_eth0_1);
+    g_assert(arr);
+    g_assert_cmpint(arr->len, ==, 1);
+
+    ac = nm_device_get_active_connection(dev_eth0_1);
+    g_assert(NM_IS_ACTIVE_CONNECTION(ac));
+
+    g_signal_connect(client, "device-removed", G_CALLBACK(_client_dev_removed), &call_count);
+
+    if (teardown_service) {
+        g_signal_connect(client,
+                         "notify::" NM_CLIENT_NM_RUNNING,
+                         G_CALLBACK(nm_running_changed),
+                         NULL);
+        nm_clear_pointer(&sinfo, nmtstc_service_cleanup);
+        nmtst_main_loop_run(gl.loop, 1000);
+        g_assert_cmpint(call_count, ==, 2);
+    } else {
+        g_clear_object(&client);
+        g_assert_cmpint(call_count, ==, 0);
+    }
+
+    if (take_ref) {
+        arr = nm_device_get_available_connections(dev_eth0_1);
+        g_assert(arr);
+        g_assert_cmpint(arr->len, ==, 0);
+
+        g_assert(!nm_device_get_active_connection(dev_eth0_1));
+
+        g_object_unref(dev_eth0_1);
     }
 }
 
@@ -1603,7 +1611,13 @@ main(int argc, char **argv)
     g_test_add_func("/libnm/devices-array", test_devices_array);
     g_test_add_func("/libnm/client-nm-running", test_client_nm_running);
     g_test_add_func("/libnm/active-connections", test_active_connections);
-    g_test_add_func("/libnm/activate-virtual", test_activate_virtual);
+    g_test_add_func("/libnm/activate-virtual/without-teardown", test_activate_virtual);
+    g_test_add_data_func("/libnm/activate-virtual/with-teardown/service",
+                         GINT_TO_POINTER(true),
+                         test_activate_virtual_teardown);
+    g_test_add_data_func("/libnm/activate-virtual/with-teardown/client",
+                         GINT_TO_POINTER(false),
+                         test_activate_virtual_teardown);
     g_test_add_func("/libnm/device-connection-compatibility", test_device_connection_compatibility);
     g_test_add_func("/libnm/connection/invalid", test_connection_invalid);
     g_test_add_func("/libnm/test_client_wait_shutdown", test_client_wait_shutdown);
diff --git a/src/nmcli/general.c b/src/nmcli/general.c
index ec18b1e206..b050d2b740 100644
--- a/src/nmcli/general.c
+++ b/src/nmcli/general.c
@@ -1233,7 +1233,7 @@ networkmanager_running(NMClient *client, GParamSpec *param, NmCli *nmc)
     running = nm_client_get_nm_running(client);
     str     = nmc_colorize(&nmc->nmc_config,
                        running ? NM_META_COLOR_MANAGER_RUNNING : NM_META_COLOR_MANAGER_STOPPED,
-                       running ? _("NetworkManager has started") : _("NetworkManager has stopped"));
+                       running ? _("NetworkManager is running") : _("NetworkManager is stopped"));
     g_print("%s\n", str);
     g_free(str);
 }
@@ -1613,15 +1613,7 @@ nmc_command_func_monitor(const NMCCommand *cmd, NmCli *nmc, int argc, const char
         return;
     }
 
-    if (!nm_client_get_nm_running(nmc->client)) {
-        char *str;
-
-        str = nmc_colorize(&nmc->nmc_config,
-                           NM_META_COLOR_MANAGER_STOPPED,
-                           _("Networkmanager is not running (waiting for it)\n"));
-        g_print("%s", str);
-        g_free(str);
-    }
+    networkmanager_running(nmc->client, NULL, nmc);
 
     g_signal_connect(nmc->client,
                      "notify::" NM_CLIENT_NM_RUNNING,
diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py
index 85a09ad25c..f018fbf34a 100755
--- a/src/tests/client/test-client.py
+++ b/src/tests/client/test-client.py
@@ -110,6 +110,7 @@ import random
 import dbus.service
 import dbus.mainloop.glib
 import io
+from signal import SIGINT
 
 import gi
 
@@ -851,7 +852,7 @@ class TestNmcli(NmTestBase):
             )
 
     def call_nmcli_pexpect(self, args):
-        env = self._env()
+        env = self._env(extra_env={"NO_COLOR": "1"})
         return pexpect.spawn(
             conf.get(ENV_NM_TEST_CLIENT_NMCLI_PATH), args, timeout=5, env=env
         )
@@ -1885,6 +1886,38 @@ class TestNmcli(NmTestBase):
         nmc.expect("Connection 'ethernet' \(.*\) successfully added.")
         nmc.expect(pexpect.EOF)
 
+    @skip_without_pexpect
+    @nm_test
+    def test_monitor(self):
+        def start_mon():
+            nmc = self.call_nmcli_pexpect(["monitor"])
+            nmc.expect("NetworkManager is running")
+            return nmc
+
+        def end_mon(nmc):
+            nmc.kill(SIGINT)
+            nmc.expect(pexpect.EOF)
+
+        nmc = start_mon()
+
+        self.srv.op_AddObj("WiredDevice", iface="eth0")
+        nmc.expect("eth0: device created\r\n")
+
+        self.srv.addConnection(
+            {"connection": {"type": "802-3-ethernet", "id": "con-1"}}
+        )
+        nmc.expect("con-1: connection profile created\r\n")
+
+        end_mon(nmc)
+
+        nmc = start_mon()
+        self.srv.shutdown()
+        self.srv = None
+        nmc.expect("eth0: device removed")
+        nmc.expect("con-1: connection profile removed")
+        nmc.expect("NetworkManager is stopped")
+        end_mon(nmc)
+
 
 ###############################################################################