From a78ba1c33a40358ca52c91fc0163b7b6f898da41 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 10:33:54 +0200 Subject: [PATCH 01/13] shared: add nm_strcmp_with_data() It is like strcmp(), but has a signature suitable for GCompareDataFunc. This is necessary with nm_utils_ptrarray_find_binary_search() to search a sorted strv array. The fault is here really C, which doesn't allow inline static functions. So, you need all kinds of slightly different flavors for the same callbacks (with or without user-data). Note that glib2 internally just casts strcmp() to GCompareDataFunc ([1]), relying on the fact how arguments are passed to the function and ignoring the additional user-data argument. But I find that really ugly and probably not permissible in general C. Dunno whether POSIX would guarantee for this to work. I'd rather not do such function pointer casts. [1] https://gitlab.gnome.org/GNOME/glib/blob/0c0cf59858abb3f8464bc55f596f9fbf599ac251/glib/garray.c#L1792 --- shared/nm-glib-aux/nm-shared-utils.c | 9 +++++++++ shared/nm-glib-aux/nm-shared-utils.h | 1 + 2 files changed, 10 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 7362dfc930..74f8a9b3f5 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -806,6 +806,15 @@ _nm_utils_ascii_str_to_uint64 (const char *str, guint base, guint64 min, guint64 /*****************************************************************************/ +int +nm_strcmp_with_data (gconstpointer a, gconstpointer b, gpointer user_data) +{ + const char *s1 = a; + const char *s2 = b; + + return strcmp (s1, s2); +} + /* like nm_strcmp_p(), suitable for g_ptr_array_sort_with_data(). * g_ptr_array_sort() just casts nm_strcmp_p() to a function of different * signature. I guess, in glib there are knowledgeable people that ensure diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 3868a4938d..d1090af397 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -918,6 +918,7 @@ nm_utf8_collate0 (const char *a, const char *b) return g_utf8_collate (a, b); } +int nm_strcmp_with_data (gconstpointer a, gconstpointer b, gpointer user_data); int nm_strcmp_p_with_data (gconstpointer a, gconstpointer b, gpointer user_data); int nm_cmp_uint32_p_with_data (gconstpointer p_a, gconstpointer p_b, gpointer user_data); int nm_cmp_int2ptr_p_with_data (gconstpointer p_a, gconstpointer p_b, gpointer user_data); From 29a7bffecf0c8a5a0b62b8831c695d3cf534537d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 10:07:52 +0200 Subject: [PATCH 02/13] shared: add nm_strcmp0_p_with_data() helper --- shared/nm-glib-aux/nm-shared-utils.c | 9 +++++++++ shared/nm-glib-aux/nm-shared-utils.h | 1 + 2 files changed, 10 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 74f8a9b3f5..171e661a95 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -831,6 +831,15 @@ nm_strcmp_p_with_data (gconstpointer a, gconstpointer b, gpointer user_data) return strcmp (s1, s2); } +int +nm_strcmp0_p_with_data (gconstpointer a, gconstpointer b, gpointer user_data) +{ + const char *s1 = *((const char **) a); + const char *s2 = *((const char **) b); + + return nm_strcmp0 (s1, s2); +} + int nm_cmp_uint32_p_with_data (gconstpointer p_a, gconstpointer p_b, gpointer user_data) { diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index d1090af397..d9c430d495 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -920,6 +920,7 @@ nm_utf8_collate0 (const char *a, const char *b) int nm_strcmp_with_data (gconstpointer a, gconstpointer b, gpointer user_data); int nm_strcmp_p_with_data (gconstpointer a, gconstpointer b, gpointer user_data); +int nm_strcmp0_p_with_data (gconstpointer a, gconstpointer b, gpointer user_data); int nm_cmp_uint32_p_with_data (gconstpointer p_a, gconstpointer p_b, gpointer user_data); int nm_cmp_int2ptr_p_with_data (gconstpointer p_a, gconstpointer p_b, gpointer user_data); From 1440a3c14956327799fcf75e725b250e31d65cb4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 10:08:50 +0200 Subject: [PATCH 03/13] shared: accept %NULL strings in nm_utils_strv_sort() In particular when calling nm_utils_strv_sort() with a positive length argument, then this is not a %NULL terminated strv arrary. That may mean that it makes sense for the input array to contain %NULL strings. Use a strcmp() function that accepts %NULL too. While this is not used at the moment, I think nm_utils_strv_sort() should accept %NULL strings beause otherwise it's a possibly unexpected restriction of its API. The function should handle sensible input gracefully. --- shared/nm-glib-aux/nm-shared-utils.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 171e661a95..4c94ddb8af 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -2708,8 +2708,8 @@ fail: * @len: the number of elements in strv. If negative, * strv must be a NULL terminated array and the length * will be calculated first. If @len is a positive - * number, all first @len elements in @strv must be - * non-NULL, valid strings. + * number, @strv is allowed to contain %NULL strings + * too. * * Ascending sort of the array @strv inplace, using plain strcmp() string * comparison. @@ -2717,9 +2717,16 @@ fail: void _nm_utils_strv_sort (const char **strv, gssize len) { + GCompareDataFunc cmp; gsize l; - l = len < 0 ? (gsize) NM_PTRARRAY_LEN (strv) : (gsize) len; + if (len < 0) { + l = NM_PTRARRAY_LEN (strv); + cmp = nm_strcmp_p_with_data; + } else { + l = len; + cmp = nm_strcmp0_p_with_data; + } if (l <= 1) return; @@ -2729,7 +2736,7 @@ _nm_utils_strv_sort (const char **strv, gssize len) g_qsort_with_data (strv, l, sizeof (const char *), - nm_strcmp_p_with_data, + cmp, NULL); } From 726192c185de74d799cf7bdce5caaa26a29e6b61 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 16:04:16 +0200 Subject: [PATCH 04/13] libnm: accept %NULL for @name argument of nm_utils_is_valid_iface_name() Rejecting %NULL for a "is-a" function can be annoying. Of course, %NULL is not a valid name. But it's sufficient that the function just returns %FALSE in that case, and not assert against the input not being %NULL. Asserting might be useful to catch bugs, but rejecting %NULL as input is more cumbersome to the caller than helping with catching bugs. Something similar was also recently done for nm_utils_is_uuid(). --- libnm-core/nm-utils.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 6dc3f53dd3..68da624818 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4660,7 +4660,7 @@ nm_utils_is_valid_iface_name_utf8safe (const char *utf8safe_name) /** * nm_utils_is_valid_iface_name: - * @name: Name of interface + * @name: (allow-none): Name of interface * @error: location to store the error occurring, or %NULL to ignore * * Validate the network interface name. @@ -4669,13 +4669,20 @@ nm_utils_is_valid_iface_name_utf8safe (const char *utf8safe_name) * function in net/core/dev.c. * * Returns: %TRUE if interface name is valid, otherwise %FALSE is returned. + * + * Before 1.20, this function did not accept %NULL as @name argument. If you + * want to run against older versions of libnm, don't pass %NULL. */ gboolean nm_utils_is_valid_iface_name (const char *name, GError **error) { int i; - g_return_val_if_fail (name, FALSE); + if (!name) { + g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("interface name is missing")); + return FALSE; + } if (name[0] == '\0') { g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, @@ -4712,13 +4719,16 @@ nm_utils_is_valid_iface_name (const char *name, GError **error) /** * nm_utils_iface_valid_name: - * @name: Name of interface + * @name: (allow-none): Name of interface * * Validate the network interface name. * * Deprecated: 1.6: use nm_utils_is_valid_iface_name() instead, with better error reporting. * * Returns: %TRUE if interface name is valid, otherwise %FALSE is returned. + * + * Before 1.20, this function did not accept %NULL as @name argument. If you + * want to run against older versions of libnm, don't pass %NULL. */ gboolean nm_utils_iface_valid_name (const char *name) From 3a6f651a98992001047aea11f356cb7223cf1656 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 15:37:55 +0200 Subject: [PATCH 05/13] core: add and use NM_MATCH_SPEC_*_TAG defines instead of plain strings The define is better, because then we can grep for all the occurances where they are used. The plain text like "mac:" is not at all unique in our source-tree. --- src/nm-config-data.c | 2 +- src/nm-core-utils.c | 17 +++++++---------- src/nm-core-utils.h | 4 ++++ .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 6 +++--- .../plugins/ifupdown/nms-ifupdown-plugin.c | 3 ++- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 85a481f6c2..5ab3b61717 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1644,7 +1644,7 @@ set_property (GObject *object, && nm_utils_hwaddr_valid (value_arr[i], -1) && nm_utils_strv_find_first (value_arr, i, value_arr[i]) < 0) { priv->no_auto_default.arr[j++] = g_strdup (value_arr[i]); - priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, g_strdup_printf ("mac:%s", value_arr[i])); + priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", value_arr[i])); } } priv->no_auto_default.arr[j++] = NULL; diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 12780e1875..d896d4d33d 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1158,13 +1158,10 @@ nm_utils_read_link_absolute (const char *link_file, GError **error) /*****************************************************************************/ -#define MAC_TAG "mac:" -#define INTERFACE_NAME_TAG "interface-name:" -#define DEVICE_TYPE_TAG "type:" -#define DRIVER_TAG "driver:" -#define SUBCHAN_TAG "s390-subchannels:" -#define DHCP_PLUGIN_TAG "dhcp-plugin:" -#define EXCEPT_TAG "except:" +#define DEVICE_TYPE_TAG "type:" +#define DRIVER_TAG "driver:" +#define DHCP_PLUGIN_TAG "dhcp-plugin:" +#define EXCEPT_TAG "except:" #define MATCH_TAG_CONFIG_NM_VERSION "nm-version:" #define MATCH_TAG_CONFIG_NM_VERSION_MIN "nm-version-min:" #define MATCH_TAG_CONFIG_NM_VERSION_MAX "nm-version-max:" @@ -1363,10 +1360,10 @@ match_device_eval (const char *spec_str, && nm_streq (spec_str, match_data->device_type); } - if (_MATCH_CHECK (spec_str, MAC_TAG)) + if (_MATCH_CHECK (spec_str, NM_MATCH_SPEC_MAC_TAG)) return match_device_hwaddr_eval (spec_str, match_data); - if (_MATCH_CHECK (spec_str, INTERFACE_NAME_TAG)) { + if (_MATCH_CHECK (spec_str, NM_MATCH_SPEC_INTERFACE_NAME_TAG)) { gboolean use_pattern = FALSE; if (spec_str[0] == '=') @@ -1418,7 +1415,7 @@ match_device_eval (const char *spec_str, match_data->driver_version ?: ""); } - if (_MATCH_CHECK (spec_str, SUBCHAN_TAG)) + if (_MATCH_CHECK (spec_str, NM_MATCH_SPEC_S390_SUBCHANNELS_TAG)) return match_data_s390_subchannels_eval (spec_str, match_data); if (_MATCH_CHECK (spec_str, DHCP_PLUGIN_TAG)) diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 1f4420ea52..855a5d080e 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -216,6 +216,10 @@ const char *nm_utils_find_helper (const char *progname, char *nm_utils_read_link_absolute (const char *link_file, GError **error); +#define NM_MATCH_SPEC_MAC_TAG "mac:" +#define NM_MATCH_SPEC_S390_SUBCHANNELS_TAG "s390-subchannels:" +#define NM_MATCH_SPEC_INTERFACE_NAME_TAG "interface-name:" + typedef enum { NM_MATCH_SPEC_NO_MATCH = 0, NM_MATCH_SPEC_MATCH = 1, diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 177c60ff4a..cf0994c323 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -5630,21 +5630,21 @@ create_unhandled_connection (const char *filename, shvarFile *ifcfg, if (v) { gs_free char *lower = g_ascii_strdown (v, -1); - *out_spec = g_strdup_printf ("%s:mac:%s", type, lower); + *out_spec = g_strdup_printf ("%s:"NM_MATCH_SPEC_MAC_TAG"%s", type, lower); return connection; } nm_clear_g_free (&value); v = svGetValueStr (ifcfg, "SUBCHANNELS", &value); if (v) { - *out_spec = g_strdup_printf ("%s:s390-subchannels:%s", type, v); + *out_spec = g_strdup_printf ("%s:"NM_MATCH_SPEC_S390_SUBCHANNELS_TAG"%s", type, v); return connection; } nm_clear_g_free (&value); v = svGetValueStr (ifcfg, "DEVICE", &value); if (v) { - *out_spec = g_strdup_printf ("%s:interface-name:%s", type, v); + *out_spec = g_strdup_printf ("%s:"NM_MATCH_SPEC_INTERFACE_NAME_TAG"%s", type, v); return connection; } diff --git a/src/settings/plugins/ifupdown/nms-ifupdown-plugin.c b/src/settings/plugins/ifupdown/nms-ifupdown-plugin.c index 94426d8b60..93f1813cfd 100644 --- a/src/settings/plugins/ifupdown/nms-ifupdown-plugin.c +++ b/src/settings/plugins/ifupdown/nms-ifupdown-plugin.c @@ -25,6 +25,7 @@ #include "nms-ifupdown-plugin.h" #include "nm-core-internal.h" +#include "nm-core-utils.h" #include "nm-config.h" #include "settings/nm-settings-plugin.h" #include "settings/nm-settings-storage.h" @@ -208,7 +209,7 @@ _unmanaged_specs (GHashTable *eni_ifaces) keys = nm_utils_strdict_get_keys (eni_ifaces, TRUE, &len); for (i = len; i > 0; ) { i--; - specs = g_slist_prepend (specs, g_strdup_printf ("interface-name:=%s", keys[i])); + specs = g_slist_prepend (specs, g_strdup_printf (NM_MATCH_SPEC_INTERFACE_NAME_TAG"=%s", keys[i])); } return specs; } From b0cb2966ed949ee21859194c695aa971f0002ddf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 15:42:30 +0200 Subject: [PATCH 06/13] ifcfg-rh: don't allow globbing for unhandled device specs With plain "interface-name:$IFNAME" globbing is enabled. So this behaves wrong if there are special characters like '*' or '?'. Also, it behaves wrong if the first character of the interface name happens to be '='. Make an explicit match. --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index cf0994c323..900a3fc1de 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -5644,7 +5644,7 @@ create_unhandled_connection (const char *filename, shvarFile *ifcfg, nm_clear_g_free (&value); v = svGetValueStr (ifcfg, "DEVICE", &value); if (v) { - *out_spec = g_strdup_printf ("%s:"NM_MATCH_SPEC_INTERFACE_NAME_TAG"%s", type, v); + *out_spec = g_strdup_printf ("%s:"NM_MATCH_SPEC_INTERFACE_NAME_TAG"=%s", type, v); return connection; } diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 9792e20ecd..45e90b919c 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -721,7 +721,7 @@ test_read_unmanaged_unrecognized (void) connection = _connection_from_file (TEST_IFCFG_DIR"/ifcfg-test-nm-controlled-unrecognized", NULL, NULL, &unhandled_spec); - g_assert_cmpstr (unhandled_spec, ==, "unmanaged:interface-name:ipoac0"); + g_assert_cmpstr (unhandled_spec, ==, "unmanaged:interface-name:=ipoac0"); /* ===== CONNECTION SETTING ===== */ s_con = nm_connection_get_setting_connection (connection); From d6b84294de17787e7b5f3210acdf58b9ad488b6b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 14:49:56 +0200 Subject: [PATCH 07/13] config: use nm_utils_g_slist_strlist_cmp() in nm_config_data_diff() --- src/nm-config-data.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 5ab3b61717..5c2232757b 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1503,16 +1503,6 @@ _match_section_infos_construct (GKeyFile *keyfile, const char *prefix) /*****************************************************************************/ -static gboolean -_slist_str_equals (GSList *a, GSList *b) -{ - while (a && b && g_strcmp0 (a->data, b->data) == 0) { - a = a->next; - b = b->next; - } - return !a && !b; -} - NMConfigChangeFlags nm_config_data_diff (NMConfigData *old_data, NMConfigData *new_data) { @@ -1541,8 +1531,8 @@ nm_config_data_diff (NMConfigData *old_data, NMConfigData *new_data) || g_strcmp0 (nm_config_data_get_connectivity_response (old_data), nm_config_data_get_connectivity_response (new_data))) changes |= NM_CONFIG_CHANGE_CONNECTIVITY; - if ( !_slist_str_equals (priv_old->no_auto_default.specs, priv_new->no_auto_default.specs) - || !_slist_str_equals (priv_old->no_auto_default.specs_config, priv_new->no_auto_default.specs_config)) + if ( nm_utils_g_slist_strlist_cmp (priv_old->no_auto_default.specs, priv_new->no_auto_default.specs) != 0 + || nm_utils_g_slist_strlist_cmp (priv_old->no_auto_default.specs_config, priv_new->no_auto_default.specs_config) != 0) changes |= NM_CONFIG_CHANGE_NO_AUTO_DEFAULT; if (g_strcmp0 (nm_config_data_get_dns_mode (old_data), nm_config_data_get_dns_mode (new_data))) From f13454cb1c503a8b35fb3c4b241c4cfb9aabf226 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 13:21:18 +0200 Subject: [PATCH 08/13] device: move check for no-auto-default to "nm-settings.c" nm_config_set_no_auto_default_for_device() is called by NMSettings, so it makes sense that also NMSettings checks whether the device is blocked. Of course, there is little difference in practice. The only downside is that most device types don't implement new_default_connection(). So the previous form performed the cheaper check first. On the other hand, we do expect to have profiles for the devices anyway. --- src/devices/nm-device.c | 3 --- src/settings/nm-settings.c | 6 +++++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4a4fd13b36..04a94db8a1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -4796,9 +4796,6 @@ nm_device_new_default_connection (NMDevice *self) if (!NM_DEVICE_GET_CLASS (self)->new_default_connection) return NULL; - if (nm_config_get_no_auto_default_for_device (nm_config_get (), self)) - return NULL; - connection = NM_DEVICE_GET_CLASS (self)->new_default_connection (self); if (!connection) return NULL; diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 13071ec367..e24814b1d8 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -2907,6 +2907,7 @@ static void device_realized (NMDevice *device, GParamSpec *pspec, NMSettings *self) { gs_unref_object NMConnection *connection = NULL; + NMSettingsPrivate *priv; NMSettingsConnection *added; GError *error = NULL; @@ -2917,12 +2918,15 @@ device_realized (NMDevice *device, GParamSpec *pspec, NMSettings *self) G_CALLBACK (device_realized), self); + priv = NM_SETTINGS_GET_PRIVATE (self); + /* If the device isn't managed or it already has a default wired connection, * ignore it. */ if ( !nm_device_get_managed (device, FALSE) || g_object_get_qdata (G_OBJECT (device), _default_wired_connection_quark ()) - || have_connection_for_device (self, device)) + || have_connection_for_device (self, device) + || nm_config_get_no_auto_default_for_device (priv->config, device)) return; connection = nm_device_new_default_connection (device); From 8437cd0895ce6e7af4bf17f7dbf17f09469e2a99 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 13:42:28 +0200 Subject: [PATCH 09/13] device,config: don't write fake MAC address to "no-auto-default.state" file For one, nm_config_get_no_auto_default_for_device() uses nm_device_spec_match_list(). This ignores fake MAC addresses. Maybe it should not do that, but it's also not clear what it would mean for the function to consider them. As such, it makes not sense trying to persist such MAC addresses to "/var/lib/NetworkManager/no-auto-default.state". For the moment, just do nothing. This still leaves the problem how we prevent the device from generating a auto-default connection. But this patch is no change in behavior, because it didn't work anyway. --- src/nm-config.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/nm-config.c b/src/nm-config.c index bb37d08fc3..1cb98e3739 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -416,6 +416,7 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) const char *hw_address; const char *const*no_auto_default_current; GPtrArray *no_auto_default_new = NULL; + gboolean is_fake; guint i; g_return_if_fail (NM_IS_CONFIG (self)); @@ -423,10 +424,23 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) priv = NM_CONFIG_GET_PRIVATE (self); - hw_address = nm_device_get_permanent_hw_address (device); + hw_address = nm_device_get_permanent_hw_address_full (device, TRUE, &is_fake); + if (!hw_address) return; + if (is_fake) { + /* this is a problem. The MAC address is fake, it's possibly only valid + * until reboot (or even less). + * + * Also, nm_device_spec_match_list() ignores fake addresses, so even if + * we would persist it, it wouldn't work (well, maybe it should?). + * + * Anyway, let's do nothing here. NMSettings needs to remember this + * in memory. */ + return; + } + no_auto_default_current = nm_config_data_get_no_auto_default (priv->config_data); if (nm_utils_strv_find_first ((char **) no_auto_default_current, -1, hw_address) >= 0) { From 38148bb33ae5538a9bc43bf69bee0e88cb19ba44 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 15:10:58 +0200 Subject: [PATCH 10/13] config: cleanup handling no_auto_default lists --- src/nm-config-data.c | 30 ++++++++++++++++++++---------- src/nm-config.c | 38 ++++++++++++++++++++------------------ 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 5c2232757b..9cc1054f6c 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1623,20 +1623,30 @@ set_property (GObject *object, case PROP_NO_AUTO_DEFAULT: /* construct-only */ { - char **value_arr = g_value_get_boxed (value); - guint i, j = 0; + const char *const*value_arr = g_value_get_boxed (value); + gsize i, j = 0; + gsize len; - priv->no_auto_default.arr = g_new (char *, g_strv_length (value_arr) + 1); + len = NM_PTRARRAY_LEN (value_arr); + + priv->no_auto_default.arr = g_new (char *, len + 1); priv->no_auto_default.specs = NULL; - for (i = 0; value_arr && value_arr[i]; i++) { - if ( *value_arr[i] - && nm_utils_hwaddr_valid (value_arr[i], -1) - && nm_utils_strv_find_first (value_arr, i, value_arr[i]) < 0) { - priv->no_auto_default.arr[j++] = g_strdup (value_arr[i]); - priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", value_arr[i])); - } + for (i = 0; i < len; i++) { + const char *s = value_arr[i]; + + if (!s[0]) + continue; + if (!nm_utils_hwaddr_valid (s, -1)) + continue; + if (nm_utils_strv_find_first (priv->no_auto_default.arr, j, s) >= 0) + continue; + + priv->no_auto_default.arr[j++] = g_strdup (s); + priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, + g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", s)); } + nm_assert (j <= len); priv->no_auto_default.arr[j++] = NULL; priv->no_auto_default.specs = g_slist_reverse (priv->no_auto_default.specs); } diff --git a/src/nm-config.c b/src/nm-config.c index 1cb98e3739..bb86a300b9 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -349,30 +349,32 @@ nm_config_get_first_start (NMConfig *config) static char ** no_auto_default_from_file (const char *no_auto_default_file) { - GPtrArray *no_auto_default_new; - char **list; - guint i; - char *data; - - no_auto_default_new = g_ptr_array_new (); + gs_free const char **list = NULL; + gs_free char *data = NULL; + gsize l = 0; + gsize i; if ( no_auto_default_file - && g_file_get_contents (no_auto_default_file, &data, NULL, NULL)) { - list = g_strsplit (data, "\n", -1); + && g_file_get_contents (no_auto_default_file, &data, NULL, NULL)) + list = nm_utils_strsplit_set (data, "\n"); + + if (list) { for (i = 0; list[i]; i++) { - if ( *list[i] - && nm_utils_hwaddr_valid (list[i], -1) - && nm_utils_strv_find_first (list, i, list[i]) < 0) - g_ptr_array_add (no_auto_default_new, list[i]); - else - g_free (list[i]); + const char *s = list[i]; + + if (!s[0]) + continue; + if (!nm_utils_hwaddr_valid (s, -1)) + continue; + + if (nm_utils_strv_find_first ((char **) list, l, s) >= 0) + continue; + + list[l++] = s; } - g_free (list); - g_free (data); } - g_ptr_array_add (no_auto_default_new, NULL); - return (char **) g_ptr_array_free (no_auto_default_new, FALSE); + return nm_utils_strv_dup (list, l); } static gboolean From c43a32ea5f4e149cf1337711c67e3e58d75e72ba Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 15:30:28 +0200 Subject: [PATCH 11/13] config: backslash escape values (utf8safe) in "no-auto-default.state" file Currently "no-auto-default.state" contains only MAC addresses in ASCII representation. Next, we will also want to write there interface names. Interface names in Linux don't enforce any encoding, so they can contain almost all characters (except NUL and '/'). In particular '\n', which we use as line separator. If we want to store there interface names, we need to properly escape and unescape them. Use our nm_utils_str_utf8safe_*() API for that. --- src/nm-config.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/nm-config.c b/src/nm-config.c index bb86a300b9..21f4c21b06 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -360,21 +360,32 @@ no_auto_default_from_file (const char *no_auto_default_file) if (list) { for (i = 0; list[i]; i++) { + gs_free char *s_to_free = NULL; const char *s = list[i]; if (!s[0]) continue; + + s = nm_utils_str_utf8safe_unescape (s, &s_to_free); + if (!nm_utils_hwaddr_valid (s, -1)) continue; if (nm_utils_strv_find_first ((char **) list, l, s) >= 0) continue; - list[l++] = s; + list[l++] = g_steal_pointer (&s_to_free) + ?: g_strdup (s); } } - return nm_utils_strv_dup (list, l); + if (l == 0) + return NULL; + + /* the strings from [0..l-1] are already cloned. We just need to + * clone the outer list (and NULL terminate). */ + list[l] = NULL; + return nm_memdup (list, sizeof (list[0]) * (l + 1)); } static gboolean @@ -386,7 +397,14 @@ no_auto_default_to_file (const char *no_auto_default_file, const char *const*no_ data = g_string_new (""); for (i = 0; no_auto_default && no_auto_default[i]; i++) { - g_string_append (data, no_auto_default[i]); + gs_free char *s_to_free = NULL; + const char *s = no_auto_default[i]; + + s = nm_utils_str_utf8safe_escape (s, + NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL + | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII, + &s_to_free); + g_string_append (data, s); g_string_append_c (data, '\n'); } success = g_file_set_contents (no_auto_default_file, data->str, data->len, error); From fb8d1cda94a3f2c27b091ac2f3696080654d25ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 18 Jul 2019 15:42:37 +0200 Subject: [PATCH 12/13] device,config: for virtual devices store the interface name to "no-auto-default.state" For devices that have no real MAC address (virtual devices) it makes no sense to store the MAC address to "no-auto-default.state" file. Also, because we later would not match the MAC address during nm_match_spec_device(). Instead, extend the format and add a "interface-name:=$IFACE" match-spec. Maybe we generally should prefer the interface-name over the MAC address. Anyway, for now, just extend the previously non-working case. --- src/nm-config-data.c | 19 ++++++++++++++++--- src/nm-config.c | 45 ++++++++++++++++++++++++++++---------------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 9cc1054f6c..77368b4dad 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1634,17 +1634,30 @@ set_property (GObject *object, for (i = 0; i < len; i++) { const char *s = value_arr[i]; + gboolean is_mac; + char *spec; if (!s[0]) continue; - if (!nm_utils_hwaddr_valid (s, -1)) + + if (NM_STR_HAS_PREFIX (s, NM_MATCH_SPEC_INTERFACE_NAME_TAG"=")) + is_mac = FALSE; + else if (nm_utils_hwaddr_valid (s, -1)) + is_mac = TRUE; + else { + /* we drop all lines that we don't understand. */ continue; + } + if (nm_utils_strv_find_first (priv->no_auto_default.arr, j, s) >= 0) continue; + spec = is_mac + ? g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", s) + : g_strdup (s); + priv->no_auto_default.arr[j++] = g_strdup (s); - priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, - g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", s)); + priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, spec); } nm_assert (j <= len); priv->no_auto_default.arr[j++] = NULL; diff --git a/src/nm-config.c b/src/nm-config.c index 21f4c21b06..f92e5a5846 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -368,8 +368,14 @@ no_auto_default_from_file (const char *no_auto_default_file) s = nm_utils_str_utf8safe_unescape (s, &s_to_free); - if (!nm_utils_hwaddr_valid (s, -1)) + if ( !NM_STR_HAS_PREFIX (s, NM_MATCH_SPEC_INTERFACE_NAME_TAG"=") + && !nm_utils_hwaddr_valid (s, -1)) { + /* Maybe we shouldn't pre-validate the device specs that we read + * from the file. After all, nm_match_spec_*() API silently ignores + * all unknown value. However, lets just be strict here for now + * and only accept what we also write. */ continue; + } if (nm_utils_strv_find_first ((char **) list, l, s) >= 0) continue; @@ -433,7 +439,10 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) NMConfigPrivate *priv; GError *error = NULL; NMConfigData *new_data = NULL; + gs_free char *spec_to_free = NULL; + const char *ifname; const char *hw_address; + const char *spec; const char *const*no_auto_default_current; GPtrArray *no_auto_default_new = NULL; gboolean is_fake; @@ -446,25 +455,29 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) hw_address = nm_device_get_permanent_hw_address_full (device, TRUE, &is_fake); - if (!hw_address) - return; - - if (is_fake) { - /* this is a problem. The MAC address is fake, it's possibly only valid - * until reboot (or even less). - * - * Also, nm_device_spec_match_list() ignores fake addresses, so even if - * we would persist it, it wouldn't work (well, maybe it should?). - * - * Anyway, let's do nothing here. NMSettings needs to remember this - * in memory. */ + if (!hw_address) { + /* No MAC address, not even a fake one. We don't do anything for this device. */ return; } + if (is_fake) { + /* A fake MAC address, no point in storing it to the file. + * Also, nm_match_spec_device() would ignore fake MAC addresses. + * + * Instead, try the interface-name... */ + ifname = nm_device_get_ip_iface (device); + if (!nm_utils_is_valid_iface_name (ifname, NULL)) + return; + + spec_to_free = g_strdup_printf (NM_MATCH_SPEC_INTERFACE_NAME_TAG"=%s", ifname); + spec = spec_to_free; + } else + spec = hw_address; + no_auto_default_current = nm_config_data_get_no_auto_default (priv->config_data); - if (nm_utils_strv_find_first ((char **) no_auto_default_current, -1, hw_address) >= 0) { - /* @hw_address is already blocked. We don't have to update our in-memory representation. + if (nm_utils_strv_find_first ((char **) no_auto_default_current, -1, spec) >= 0) { + /* @spec is already blocked. We don't have to update our in-memory representation. * Maybe we should write to no_auto_default_file anew, but let's save that too. */ return; } @@ -472,7 +485,7 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) no_auto_default_new = g_ptr_array_new (); for (i = 0; no_auto_default_current && no_auto_default_current[i]; i++) g_ptr_array_add (no_auto_default_new, (char *) no_auto_default_current[i]); - g_ptr_array_add (no_auto_default_new, (char *) hw_address); + g_ptr_array_add (no_auto_default_new, (char *) spec); g_ptr_array_add (no_auto_default_new, NULL); if (!no_auto_default_to_file (priv->no_auto_default_file, (const char *const*) no_auto_default_new->pdata, &error)) { From b424f7547902eff9293b5f173e32245ec51982b5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Jul 2019 10:10:35 +0200 Subject: [PATCH 13/13] config: simplify no-auto-default list handling and sort entries - don't let no_auto_default_from_file() do any preprocessing of the lines that it reads. It merely splits the lines at '\n' and utf8safe-unescapes them. This was previously duplicated also by NMConfigData's property setter. We don't need to do it twice. - sort the lines. This makes the entire handling O(n*ln(n)) instead of O(n^2). Also, sorting effectively normalizes the content, and it's desirable to have one true representation of what we write. --- shared/nm-glib-aux/nm-shared-utils.c | 2 + src/nm-config-data.c | 36 ++++++------ src/nm-config.c | 86 +++++++++++----------------- 3 files changed, 55 insertions(+), 69 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 4c94ddb8af..c8a253a668 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -1364,6 +1364,8 @@ _nm_utils_strv_cleanup (char **strv, return strv; if (strip_whitespace) { + /* we only modify the strings pointed to by @strv if @strip_whitespace is + * requested. Otherwise, the strings themselves are untouched. */ for (i = 0; strv[i]; i++) g_strstrip (strv[i]); } diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 77368b4dad..5655c8fd0e 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -1623,23 +1623,28 @@ set_property (GObject *object, case PROP_NO_AUTO_DEFAULT: /* construct-only */ { - const char *const*value_arr = g_value_get_boxed (value); - gsize i, j = 0; + const char *const*value_arr_orig = g_value_get_boxed (value); + gs_free const char **value_arr = NULL; + GSList *specs = NULL; + gsize i, j; gsize len; + len = NM_PTRARRAY_LEN (value_arr_orig); + + /* sort entries, remove duplicates and empty words. */ + value_arr = len == 0 + ? NULL + : nm_memdup (value_arr_orig, sizeof (const char *) * (len + 1)); + nm_utils_strv_sort (value_arr, len); + _nm_utils_strv_cleanup ((char **) value_arr, FALSE, TRUE, TRUE); + len = NM_PTRARRAY_LEN (value_arr); - - priv->no_auto_default.arr = g_new (char *, len + 1); - priv->no_auto_default.specs = NULL; - + j = 0; for (i = 0; i < len; i++) { const char *s = value_arr[i]; gboolean is_mac; char *spec; - if (!s[0]) - continue; - if (NM_STR_HAS_PREFIX (s, NM_MATCH_SPEC_INTERFACE_NAME_TAG"=")) is_mac = FALSE; else if (nm_utils_hwaddr_valid (s, -1)) @@ -1649,19 +1654,16 @@ set_property (GObject *object, continue; } - if (nm_utils_strv_find_first (priv->no_auto_default.arr, j, s) >= 0) - continue; + value_arr[j++] = s; spec = is_mac ? g_strdup_printf (NM_MATCH_SPEC_MAC_TAG"%s", s) : g_strdup (s); - - priv->no_auto_default.arr[j++] = g_strdup (s); - priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, spec); + specs = g_slist_prepend (specs, spec); } - nm_assert (j <= len); - priv->no_auto_default.arr[j++] = NULL; - priv->no_auto_default.specs = g_slist_reverse (priv->no_auto_default.specs); + + priv->no_auto_default.arr = nm_utils_strv_dup (value_arr, j); + priv->no_auto_default.specs = g_slist_reverse (specs); } break; default: diff --git a/src/nm-config.c b/src/nm-config.c index f92e5a5846..d12798143f 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -349,9 +349,8 @@ nm_config_get_first_start (NMConfig *config) static char ** no_auto_default_from_file (const char *no_auto_default_file) { - gs_free const char **list = NULL; gs_free char *data = NULL; - gsize l = 0; + const char **list = NULL; gsize i; if ( no_auto_default_file @@ -359,47 +358,21 @@ no_auto_default_from_file (const char *no_auto_default_file) list = nm_utils_strsplit_set (data, "\n"); if (list) { - for (i = 0; list[i]; i++) { - gs_free char *s_to_free = NULL; - const char *s = list[i]; - - if (!s[0]) - continue; - - s = nm_utils_str_utf8safe_unescape (s, &s_to_free); - - if ( !NM_STR_HAS_PREFIX (s, NM_MATCH_SPEC_INTERFACE_NAME_TAG"=") - && !nm_utils_hwaddr_valid (s, -1)) { - /* Maybe we shouldn't pre-validate the device specs that we read - * from the file. After all, nm_match_spec_*() API silently ignores - * all unknown value. However, lets just be strict here for now - * and only accept what we also write. */ - continue; - } - - if (nm_utils_strv_find_first ((char **) list, l, s) >= 0) - continue; - - list[l++] = g_steal_pointer (&s_to_free) - ?: g_strdup (s); - } + for (i = 0; list[i]; i++) + list[i] = nm_utils_str_utf8safe_unescape_cp (list[i]); } - if (l == 0) - return NULL; - - /* the strings from [0..l-1] are already cloned. We just need to - * clone the outer list (and NULL terminate). */ - list[l] = NULL; - return nm_memdup (list, sizeof (list[0]) * (l + 1)); + /* The returned buffer here is not at all compact. That means, it has additional + * memory allocations and is larger than needed. That means, you should not keep + * this result around, only process it further and free it. */ + return (char **) list; } static gboolean no_auto_default_to_file (const char *no_auto_default_file, const char *const*no_auto_default, GError **error) { - GString *data; - gboolean success; - guint i; + nm_auto_free_gstring GString *data = NULL; + gsize i; data = g_string_new (""); for (i = 0; no_auto_default && no_auto_default[i]; i++) { @@ -413,9 +386,7 @@ no_auto_default_to_file (const char *no_auto_default_file, const char *const*no_ g_string_append (data, s); g_string_append_c (data, '\n'); } - success = g_file_set_contents (no_auto_default_file, data->str, data->len, error); - g_string_free (data, TRUE); - return success; + return g_file_set_contents (no_auto_default_file, data->str, data->len, error); } gboolean @@ -444,9 +415,10 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) const char *hw_address; const char *spec; const char *const*no_auto_default_current; - GPtrArray *no_auto_default_new = NULL; + gs_free const char **no_auto_default_new = NULL; gboolean is_fake; - guint i; + gsize len; + gssize idx; g_return_if_fail (NM_IS_CONFIG (self)); g_return_if_fail (NM_IS_DEVICE (device)); @@ -476,28 +448,38 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) no_auto_default_current = nm_config_data_get_no_auto_default (priv->config_data); - if (nm_utils_strv_find_first ((char **) no_auto_default_current, -1, spec) >= 0) { + len = NM_PTRARRAY_LEN (no_auto_default_current); + + idx = nm_utils_ptrarray_find_binary_search ((gconstpointer *) no_auto_default_current, + len, + spec, + nm_strcmp_with_data, + NULL, + NULL, + NULL); + if (idx >= 0) { /* @spec is already blocked. We don't have to update our in-memory representation. * Maybe we should write to no_auto_default_file anew, but let's save that too. */ return; } - no_auto_default_new = g_ptr_array_new (); - for (i = 0; no_auto_default_current && no_auto_default_current[i]; i++) - g_ptr_array_add (no_auto_default_new, (char *) no_auto_default_current[i]); - g_ptr_array_add (no_auto_default_new, (char *) spec); - g_ptr_array_add (no_auto_default_new, NULL); + idx = ~idx; - if (!no_auto_default_to_file (priv->no_auto_default_file, (const char *const*) no_auto_default_new->pdata, &error)) { + no_auto_default_new = g_new (const char *, len + 2); + if (idx > 0) + memcpy (no_auto_default_new, no_auto_default_current, sizeof (const char *) * idx); + no_auto_default_new[idx] = spec; + if (idx < len) + memcpy (&no_auto_default_new[idx + 1], &no_auto_default_current[idx], sizeof (const char *) * (len - idx)); + no_auto_default_new[len + 1] = NULL; + + if (!no_auto_default_to_file (priv->no_auto_default_file, no_auto_default_new, &error)) { _LOGW ("Could not update no-auto-default.state file: %s", error->message); g_error_free (error); } - new_data = nm_config_data_new_update_no_auto_default (priv->config_data, (const char *const*) no_auto_default_new->pdata); - - /* unref no_auto_default_set here. Note that _set_config_data() probably invalidates the content of the array. */ - g_ptr_array_unref (no_auto_default_new); + new_data = nm_config_data_new_update_no_auto_default (priv->config_data, no_auto_default_new); _set_config_data (self, new_data, NM_CONFIG_CHANGE_CAUSE_NO_AUTO_DEFAULT); }