From fe2eddd67c0c85c22a87b685ab6265c1fb4887af Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 10 Nov 2022 12:59:51 +0100 Subject: [PATCH 1/5] nmcli/monitor: always print running status on monitor startup Previously we'd note if NM is stopped, but not if it's running. I suppose it's nice for the user to know that the monitor started running, but, it's also important for the monitor to be testable (so that we know that we are ready to start adding mock objects, etc.) This also gets rids of some duplication at expense of a little less nuanced message. --- src/nmcli/general.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) 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, From 253bb3579f1c9439fbe8b81b55b4e97310b523c1 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 10 Nov 2022 12:52:17 +0100 Subject: [PATCH 2/5] tests/client: test nmcli monitor Some basic tests for nmcli monitor. Note that they now assert against behavior that I find incorrect. Will be fixed separately. --- src/tests/client/test-client.py | 38 ++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 85a09ad25c..4ee64ecb86 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,41 @@ 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("\(null\): device removed") + nmc.expect("con-1: connection profile removed") + nmc.expect("Hostname set to '\(null\)'") + nmc.expect("Networkmanager is now in the 'unknown' state") + nmc.expect("Connectivity is now 'unknown'") + nmc.expect("NetworkManager is stopped") + end_mon(nmc) + ############################################################################### From d3490a92db2f74a457491d0275778741bf78e8d6 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 11 Nov 2022 22:19:41 +0100 Subject: [PATCH 3/5] libnm/client/test: always run the cleanup test The part where a device was created and its cleanup on client description was only run randomly. This is silly and gave me hard time. No reason not to be always running it. --- src/libnm-client-impl/tests/test-nm-client.c | 184 ++++++++++--------- 1 file changed, 101 insertions(+), 83 deletions(-) diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c index 07b0789b55..de43daa18e 100644 --- a/src/libnm-client-impl/tests/test-nm-client.c +++ b/src/libnm-client-impl/tests/test-nm-client.c @@ -791,50 +791,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 +846,107 @@ 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 +_dev_eth0_1_state_changed_cb(NMDevice *device, + NMDeviceState new_state, + NMDeviceState old_state, + NMDeviceStateReason reason, + int *p_count_call) +{ + 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_teardown(void) +{ + nmtstc_auto_service_cleanup NMTstcServiceInfo *sinfo = NULL; + gs_unref_object NMClient *client = NULL; + NMDevice *dev_eth0_1; + NMActiveConnection *ac; + const GPtrArray *arr; + gulong sig_id; + int call_count = 0; + gboolean take_ref = nmtst_get_rand_bool(); + + if (nmtst_test_skip_slow()) + return; + + sinfo = nmtstc_service_init(); + if (!nmtstc_service_available(sinfo)) + return; + + client = _activate_virtual(sinfo); + + /* ensure we got all the necessary events in place. */ + nmtst_main_loop_run(gl.loop, 50); + + 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); + + 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)); + + sig_id = g_signal_connect(dev_eth0_1, + "state-changed", + G_CALLBACK(_dev_eth0_1_state_changed_cb), + &call_count); + + g_clear_object(&client); + + g_assert_cmpint(call_count, ==, 1); + + if (take_ref) { arr = nm_device_get_available_connections(dev_eth0_1); g_assert(arr); - g_assert_cmpint(arr->len, ==, 1); + g_assert_cmpint(arr->len, ==, 0); - ac = nm_device_get_active_connection(dev_eth0_1); - g_assert(NM_IS_ACTIVE_CONNECTION(ac)); + g_assert(!nm_device_get_active_connection(dev_eth0_1)); - sig_id = g_signal_connect(dev_eth0_1, - "state-changed", - G_CALLBACK(_dev_eth0_1_state_changed_cb), - &call_count); + nm_clear_g_signal_handler(dev_eth0_1, &sig_id); - g_clear_object(&client); - - g_assert_cmpint(call_count, ==, 1); - - 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)); - - nm_clear_g_signal_handler(dev_eth0_1, &sig_id); - - g_object_unref(dev_eth0_1); - } + g_object_unref(dev_eth0_1); } } @@ -1603,7 +1620,8 @@ 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_func("/libnm/activate-virtual/with-teardown", 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); From 8b3cb0dabf5e8a04d9a8e6fa0e67acf327034cad Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 11 Nov 2022 22:40:17 +0100 Subject: [PATCH 4/5] libnm/client/test: test cleanup on service shutdown Currently we assert that properties are reset on client teardown. That is not the right thing to do and we're not going to do that in future. However, what is important to test is that the properties are reset when the daemon goes away. Test that. --- src/libnm-client-impl/tests/test-nm-client.c | 22 ++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c index de43daa18e..c97d9e263f 100644 --- a/src/libnm-client-impl/tests/test-nm-client.c +++ b/src/libnm-client-impl/tests/test-nm-client.c @@ -891,9 +891,8 @@ _dev_eth0_1_state_changed_cb(NMDevice *device, g_assert(!nm_device_get_active_connection(device)); } - static void -test_activate_virtual_teardown(void) +test_activate_virtual_teardown(gconstpointer user_data) { nmtstc_auto_service_cleanup NMTstcServiceInfo *sinfo = NULL; gs_unref_object NMClient *client = NULL; @@ -901,8 +900,9 @@ test_activate_virtual_teardown(void) NMActiveConnection *ac; const GPtrArray *arr; gulong sig_id; - int call_count = 0; - gboolean take_ref = nmtst_get_rand_bool(); + int call_count = 0; + gboolean take_ref = nmtst_get_rand_bool(); + gboolean teardown_service = GPOINTER_TO_INT(user_data); if (nmtst_test_skip_slow()) return; @@ -933,7 +933,12 @@ test_activate_virtual_teardown(void) G_CALLBACK(_dev_eth0_1_state_changed_cb), &call_count); - g_clear_object(&client); + if (teardown_service) { + nmtstc_service_cleanup(g_steal_pointer(&sinfo)); + nmtst_main_loop_run(gl.loop, 50); + } else { + g_clear_object(&client); + } g_assert_cmpint(call_count, ==, 1); @@ -1621,7 +1626,12 @@ main(int argc, char **argv) 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/without-teardown", test_activate_virtual); - g_test_add_func("/libnm/activate-virtual/with-teardown", test_activate_virtual_teardown); + 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); From aaa9a9cd25664dfd4f4e2c33d8b0c3a5d8e3df5c Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 24 Jun 2022 13:59:31 +0200 Subject: [PATCH 5/5] libnm/client: don't reset properties when interface goes away In case the D-Bus interfaces start dropping off (typically all off them go one by one when the object is being deleted), don't reset all the properties. In particular, keep most properties around, only tear down "o" and "ao", so that the object dependencies get torn down, but we still get enough properties around to identify what the dead object was its heyday. One example of where this is not good is when the device-removed signal is emmitted, the device no longer has the ifname: $ nmcli monitor (null): device removed (null): device removed ... --- src/libnm-client-impl/nm-client.c | 11 +++++- src/libnm-client-impl/tests/test-nm-client.c | 38 +++++++------------- src/tests/client/test-client.py | 5 +-- 3 files changed, 23 insertions(+), 31 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 c97d9e263f..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); } @@ -865,25 +866,12 @@ test_activate_virtual(void) } static void -_dev_eth0_1_state_changed_cb(NMDevice *device, - NMDeviceState new_state, - NMDeviceState old_state, - NMDeviceStateReason reason, - int *p_count_call) +_client_dev_removed(NMClient *client, NMDevice *device, int *p_count_call) { 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); @@ -899,7 +887,6 @@ test_activate_virtual_teardown(gconstpointer user_data) NMDevice *dev_eth0_1; NMActiveConnection *ac; const GPtrArray *arr; - gulong sig_id; int call_count = 0; gboolean take_ref = nmtst_get_rand_bool(); gboolean teardown_service = GPOINTER_TO_INT(user_data); @@ -928,20 +915,21 @@ test_activate_virtual_teardown(gconstpointer user_data) ac = nm_device_get_active_connection(dev_eth0_1); g_assert(NM_IS_ACTIVE_CONNECTION(ac)); - sig_id = g_signal_connect(dev_eth0_1, - "state-changed", - G_CALLBACK(_dev_eth0_1_state_changed_cb), - &call_count); + g_signal_connect(client, "device-removed", G_CALLBACK(_client_dev_removed), &call_count); if (teardown_service) { - nmtstc_service_cleanup(g_steal_pointer(&sinfo)); - nmtst_main_loop_run(gl.loop, 50); + 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); } - g_assert_cmpint(call_count, ==, 1); - if (take_ref) { arr = nm_device_get_available_connections(dev_eth0_1); g_assert(arr); @@ -949,8 +937,6 @@ test_activate_virtual_teardown(gconstpointer user_data) g_assert(!nm_device_get_active_connection(dev_eth0_1)); - nm_clear_g_signal_handler(dev_eth0_1, &sig_id); - g_object_unref(dev_eth0_1); } } diff --git a/src/tests/client/test-client.py b/src/tests/client/test-client.py index 4ee64ecb86..f018fbf34a 100755 --- a/src/tests/client/test-client.py +++ b/src/tests/client/test-client.py @@ -1913,11 +1913,8 @@ class TestNmcli(NmTestBase): nmc = start_mon() self.srv.shutdown() self.srv = None - nmc.expect("\(null\): device removed") + nmc.expect("eth0: device removed") nmc.expect("con-1: connection profile removed") - nmc.expect("Hostname set to '\(null\)'") - nmc.expect("Networkmanager is now in the 'unknown' state") - nmc.expect("Connectivity is now 'unknown'") nmc.expect("NetworkManager is stopped") end_mon(nmc)