From ad9e7488167ab25a5915040e813e76a5b669257b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Mar 2020 13:21:48 +0100 Subject: [PATCH 1/4] core: cleanup nm_config_device_state_prune_unseen() and accept NMPlatform for skipping pruning --- src/nm-config.c | 36 +++++++++++++++++++++++------------- src/nm-config.h | 3 ++- src/nm-manager.c | 8 ++++---- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/nm-config.c b/src/nm-config.c index 4b4ffd312..49b61d5bd 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -2299,6 +2299,8 @@ _config_device_state_data_new (int ifindex, GKeyFile *kf) return device_state; } +#define DEVICE_STATE_FILENAME_LEN_MAX 60 + /** * nm_config_device_state_load: * @ifindex: the ifindex for which the state is to load @@ -2310,7 +2312,7 @@ NMConfigDeviceStateData * nm_config_device_state_load (int ifindex) { NMConfigDeviceStateData *device_state; - char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR) + 60]; + char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR"/") + DEVICE_STATE_FILENAME_LEN_MAX + 1]; gs_unref_keyfile GKeyFile *kf = NULL; const char *nm_owned_str; @@ -2394,7 +2396,7 @@ nm_config_device_state_write (int ifindex, const char *next_server, const char *root_path) { - char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR) + 60]; + char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR"/") + DEVICE_STATE_FILENAME_LEN_MAX + 1]; GError *local = NULL; gs_unref_keyfile GKeyFile *kf = NULL; @@ -2477,35 +2479,43 @@ nm_config_device_state_write (int ifindex, } void -nm_config_device_state_prune_unseen (GHashTable *seen_ifindexes) +nm_config_device_state_prune_unseen (GHashTable *preserve_ifindexes, + NMPlatform *preserve_in_platform) { GDir *dir; const char *fn; - int ifindex; - gsize fn_len; - char buf[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR"/") + 30 + 3] = NM_CONFIG_DEVICE_STATE_DIR"/"; + char buf[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR"/") + DEVICE_STATE_FILENAME_LEN_MAX + 1] = NM_CONFIG_DEVICE_STATE_DIR"/"; char *buf_p = &buf[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR"/")]; - g_return_if_fail (seen_ifindexes); - dir = g_dir_open (NM_CONFIG_DEVICE_STATE_DIR, 0, NULL); if (!dir) return; while ((fn = g_dir_read_name (dir))) { + int ifindex; + gsize fn_len; + ifindex = _device_state_parse_filename (fn); if (ifindex <= 0) continue; - if (g_hash_table_contains (seen_ifindexes, GINT_TO_POINTER (ifindex))) + + if ( preserve_ifindexes + && g_hash_table_contains (preserve_ifindexes, GINT_TO_POINTER (ifindex))) continue; - fn_len = strlen (fn) + 1; + if ( preserve_in_platform + && nm_platform_link_get (preserve_in_platform, ifindex)) + continue; + + fn_len = strlen (fn); + nm_assert (fn_len > 0); nm_assert (&buf_p[fn_len] < &buf[G_N_ELEMENTS (buf)]); - memcpy (buf_p, fn, fn_len); + memcpy (buf_p, fn, fn_len + 1u); nm_assert (({ char bb[30]; - nm_sprintf_buf (bb, "%d", ifindex); - nm_streq0 (bb, buf_p); + + nm_streq0 (nm_sprintf_buf (bb, "%d", ifindex), + buf_p); })); _LOGT ("device-state: prune #%d (%s)", ifindex, buf); (void) unlink (buf); diff --git a/src/nm-config.h b/src/nm-config.h index d9460ebb4..048d64f41 100644 --- a/src/nm-config.h +++ b/src/nm-config.h @@ -258,7 +258,8 @@ gboolean nm_config_device_state_write (int ifindex, const char *next_server, const char *root_path); -void nm_config_device_state_prune_unseen (GHashTable *seen_ifindexes); +void nm_config_device_state_prune_unseen (GHashTable *preserve_ifindexes, + NMPlatform *preserve_in_platform); const GHashTable *nm_config_device_state_get_all (NMConfig *self); const NMConfigDeviceStateData *nm_config_device_state_get (NMConfig *self, diff --git a/src/nm-manager.c b/src/nm-manager.c index e49c739a1..b9a1b5bbc 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6555,19 +6555,19 @@ void nm_manager_write_device_state_all (NMManager *self) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - gs_unref_hashtable GHashTable *seen_ifindexes = NULL; + gs_unref_hashtable GHashTable *preserve_ifindexes = NULL; NMDevice *device; - seen_ifindexes = g_hash_table_new (nm_direct_hash, NULL); + preserve_ifindexes = g_hash_table_new (nm_direct_hash, NULL); c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { if (nm_manager_write_device_state (self, device)) { - g_hash_table_add (seen_ifindexes, + g_hash_table_add (preserve_ifindexes, GINT_TO_POINTER (nm_device_get_ip_ifindex (device))); } } - nm_config_device_state_prune_unseen (seen_ifindexes); + nm_config_device_state_prune_unseen (preserve_ifindexes, NULL); } static gboolean From ecb0210e7a225f2b73149229cc96ac84f93404d1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Mar 2020 16:52:57 +0100 Subject: [PATCH 2/4] core/trivial: rename nm_config_device_state_prune_unseen() to nm_config_device_state_prune_stale() It's just a more fitting name after the previous change. --- src/nm-config.c | 4 ++-- src/nm-config.h | 4 ++-- src/nm-manager.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/nm-config.c b/src/nm-config.c index 49b61d5bd..558dab7ae 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -2479,8 +2479,8 @@ nm_config_device_state_write (int ifindex, } void -nm_config_device_state_prune_unseen (GHashTable *preserve_ifindexes, - NMPlatform *preserve_in_platform) +nm_config_device_state_prune_stale (GHashTable *preserve_ifindexes, + NMPlatform *preserve_in_platform) { GDir *dir; const char *fn; diff --git a/src/nm-config.h b/src/nm-config.h index 048d64f41..b4478ceb0 100644 --- a/src/nm-config.h +++ b/src/nm-config.h @@ -258,8 +258,8 @@ gboolean nm_config_device_state_write (int ifindex, const char *next_server, const char *root_path); -void nm_config_device_state_prune_unseen (GHashTable *preserve_ifindexes, - NMPlatform *preserve_in_platform); +void nm_config_device_state_prune_stale (GHashTable *preserve_ifindexes, + NMPlatform *preserve_in_platform); const GHashTable *nm_config_device_state_get_all (NMConfig *self); const NMConfigDeviceStateData *nm_config_device_state_get (NMConfig *self, diff --git a/src/nm-manager.c b/src/nm-manager.c index b9a1b5bbc..90a1562b5 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6567,7 +6567,7 @@ nm_manager_write_device_state_all (NMManager *self) } } - nm_config_device_state_prune_unseen (preserve_ifindexes, NULL); + nm_config_device_state_prune_stale (preserve_ifindexes, NULL); } static gboolean From 5477847eed9654727df5b70767a2a6498da1cb67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Mar 2020 13:38:49 +0100 Subject: [PATCH 3/4] core: return ifindex from nm_manager_write_device_state() nm_manager_write_device_state() writes the device state to a file. The ifindex is here important, because that is the identifier for the device and is also used as file name. Return the ifindex that was used, instead of letting the caller reimplement the knowledge which ifindex was used. --- src/nm-manager.c | 34 +++++++++++++++++++++------------- src/nm-manager.h | 2 +- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 90a1562b5..99e658f26 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1516,7 +1516,7 @@ manager_device_state_changed (NMDevice *device, NM_DEVICE_STATE_UNMANAGED, NM_DEVICE_STATE_DISCONNECTED, NM_DEVICE_STATE_ACTIVATED)) - nm_manager_write_device_state (self, device); + nm_manager_write_device_state (self, device, NULL); if (NM_IN_SET (new_state, NM_DEVICE_STATE_UNAVAILABLE, @@ -6484,7 +6484,7 @@ start_factory (NMDeviceFactory *factory, gpointer user_data) } gboolean -nm_manager_write_device_state (NMManager *self, NMDevice *device) +nm_manager_write_device_state (NMManager *self, NMDevice *device, int *out_ifindex) { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); int ifindex; @@ -6500,6 +6500,8 @@ nm_manager_write_device_state (NMManager *self, NMDevice *device) const char *next_server = NULL; const char *root_path = NULL; + NM_SET_OUT (out_ifindex, 0); + ifindex = nm_device_get_ip_ifindex (device); if (ifindex <= 0) return FALSE; @@ -6540,15 +6542,19 @@ nm_manager_write_device_state (NMManager *self, NMDevice *device) next_server = nm_dhcp_config_get_option (dhcp_config, "next_server"); } - return nm_config_device_state_write (ifindex, - managed_type, - perm_hw_addr_fake, - uuid, - nm_owned, - route_metric_default_aspired, - route_metric_default_effective, - next_server, - root_path); + if (!nm_config_device_state_write (ifindex, + managed_type, + perm_hw_addr_fake, + uuid, + nm_owned, + route_metric_default_aspired, + route_metric_default_effective, + next_server, + root_path)) + return FALSE; + + NM_SET_OUT (out_ifindex, ifindex); + return TRUE; } void @@ -6561,9 +6567,11 @@ nm_manager_write_device_state_all (NMManager *self) preserve_ifindexes = g_hash_table_new (nm_direct_hash, NULL); c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) { - if (nm_manager_write_device_state (self, device)) { + int ifindex; + + if (nm_manager_write_device_state (self, device, &ifindex)) { g_hash_table_add (preserve_ifindexes, - GINT_TO_POINTER (nm_device_get_ip_ifindex (device))); + GINT_TO_POINTER (ifindex)); } } diff --git a/src/nm-manager.h b/src/nm-manager.h index ad06e318f..5873abd24 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -103,7 +103,7 @@ NMSettingsConnection **nm_manager_get_activatable_connections (NMManager *manage guint *out_len); void nm_manager_write_device_state_all (NMManager *manager); -gboolean nm_manager_write_device_state (NMManager *manager, NMDevice *device); +gboolean nm_manager_write_device_state (NMManager *manager, NMDevice *device, int *out_ifindex); /* Device handling */ From 332df7a58e86ce08cfd9331897d8b928ae6d267e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Mar 2020 15:44:53 +0100 Subject: [PATCH 4/4] core: periodically cleanup unused device state files from /run Otherwise, we only prune unused files when the service terminates. Usually, NetworkManager service doesn't get restarted before shutdown of the system (nor should it be). That means, if you create (and destroy) a large number of software devices, the state files pile up. From time to time, go through the files on disk and delete those that are no longer relevant. In this case, "from time to time" means after we write/update state files 100 times. --- src/nm-manager.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 99e658f26..ee1feb941 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -50,6 +50,8 @@ #include "nm-dispatcher.h" #include "NetworkManagerUtils.h" +#define DEVICE_STATE_PRUNE_RATELIMIT_MAX 100u + /*****************************************************************************/ typedef struct { @@ -191,6 +193,8 @@ typedef struct { NMConnectivityState connectivity_state; + guint8 device_state_prune_ratelimit_count; + bool startup:1; bool devices_inited:1; @@ -1515,9 +1519,23 @@ manager_device_state_changed (NMDevice *device, if (NM_IN_SET (new_state, NM_DEVICE_STATE_UNMANAGED, NM_DEVICE_STATE_DISCONNECTED, - NM_DEVICE_STATE_ACTIVATED)) + NM_DEVICE_STATE_ACTIVATED)) { nm_manager_write_device_state (self, device, NULL); + G_STATIC_ASSERT_EXPR (DEVICE_STATE_PRUNE_RATELIMIT_MAX < G_MAXUINT8); + if (priv->device_state_prune_ratelimit_count++ > DEVICE_STATE_PRUNE_RATELIMIT_MAX) { + /* We write the device state to /run. The state files are named after the + * ifindex (which is assumed to be unique and not repeat -- in practice + * it may repeat). So from time to time, we prune device state files + * for interfaces that no longer exist. + * + * Otherwise, the files might pile up if you create (and destroy) a large + * number of software devices. */ + priv->device_state_prune_ratelimit_count = 0; + nm_config_device_state_prune_stale (NULL, priv->platform); + } + } + if (NM_IN_SET (new_state, NM_DEVICE_STATE_UNAVAILABLE, NM_DEVICE_STATE_DISCONNECTED))