From 1f906d92147af4dfdccb189c101c6d98ddd04a03 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 18:27:42 +0100 Subject: [PATCH 01/19] shared/glib: add compat implementation for g_value_unset() to allow unintialized GValue --- shared/nm-utils/nm-glib.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/shared/nm-utils/nm-glib.h b/shared/nm-utils/nm-glib.h index 770cf0fe39..b7534edd2a 100644 --- a/shared/nm-utils/nm-glib.h +++ b/shared/nm-utils/nm-glib.h @@ -538,4 +538,28 @@ _nm_g_variant_new_printf (const char *format_string, ...) /*****************************************************************************/ +#if !GLIB_CHECK_VERSION (2, 47, 1) +/* Older versions of g_value_unset() only allowed to unset a GValue which + * was initialized previously. This was relaxed ([1], [2], [3]). + * + * Our nm_auto_unset_gvalue macro requires to be able to call g_value_unset(). + * Also, it is our general practice to allow for that. Add a compat implementation. + * + * [1] https://gitlab.gnome.org/GNOME/glib/commit/4b2d92a864f1505f1b08eb639d74293fa32681da + * [2] commit "Allow passing unset GValues to g_value_unset()" + * [3] https://bugzilla.gnome.org/show_bug.cgi?id=755766 + */ +static inline void +_nm_g_value_unset (GValue *value) +{ + g_return_if_fail (value); + + if (value->g_type != 0) + g_value_unset (value); +} +#define g_value_unset _nm_g_value_unset +#endif + +/*****************************************************************************/ + #endif /* __NM_GLIB_H__ */ From e3ea8ecd33fd1815c8e219af92b807fd81b533eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 09:57:34 +0100 Subject: [PATCH 02/19] shared: add NM_STR_HAS_PREFIX() macro Commonly, the prefix is a string constant. We don't need to call g_str_has_prefix() for that, which first requires strlen() on the prefix. All the information is readily available. Add a macro for that. --- shared/nm-utils/nm-macros-internal.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 7cebd56cc0..3578a55291 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -829,6 +829,9 @@ fcn (void) \ #define nm_streq(s1, s2) (strcmp (s1, s2) == 0) #define nm_streq0(s1, s2) (g_strcmp0 (s1, s2) == 0) +#define NM_STR_HAS_PREFIX(str, prefix) \ + (strncmp ((str), ""prefix"", NM_STRLEN (prefix)) == 0) + /*****************************************************************************/ static inline GString * From b93a2cf728dddb193ef168e88ac5ea8d7185e9dd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 13:22:01 +0100 Subject: [PATCH 03/19] shared/tests: add nmtst_rand_select() test util --- shared/nm-utils/nm-test-utils.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 3216593e70..50c0abcbef 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -905,6 +905,16 @@ nmtst_rand_buf (GRand *rand, gpointer buffer, gsize buffer_length) return buffer; } +#define _nmtst_rand_select(uniq, v0, ...) \ + ({ \ + typeof (v0) NM_UNIQ_T (UNIQ, uniq)[1 + NM_NARG (__VA_ARGS__)] = { (v0), __VA_ARGS__ }; \ + \ + NM_UNIQ_T (UNIQ, uniq)[nmtst_get_rand_int () % G_N_ELEMENTS (NM_UNIQ_T (UNIQ, uniq))]; \ + }) + +#define nmtst_rand_select(...) \ + _nmtst_rand_select (NM_UNIQ, __VA_ARGS__) + static inline void * nmtst_rand_perm (GRand *rand, void *dst, const void *src, gsize elmt_size, gsize n_elmt) { From 04c6c912b0c461d122ae22c7bed002f529db0f6f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 13:22:28 +0100 Subject: [PATCH 04/19] shared/tests: add nmtst_keyfile_assert_data() test util --- shared/nm-utils/nm-test-utils.h | 58 ++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 50c0abcbef..c235d93d50 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1976,8 +1976,8 @@ nmtst_assert_hwaddr_equals (gconstpointer hwaddr1, gssize hwaddr1_len, const cha static inline NMConnection * nmtst_create_connection_from_keyfile (const char *keyfile_str, const char *full_filename) { - GKeyFile *keyfile; - GError *error = NULL; + gs_unref_keyfile GKeyFile *keyfile = NULL; + gs_free_error GError *error = NULL; gboolean success; NMConnection *con; gs_free char *filename = g_path_get_basename (full_filename); @@ -1988,14 +1988,10 @@ nmtst_create_connection_from_keyfile (const char *keyfile_str, const char *full_ keyfile = g_key_file_new (); success = g_key_file_load_from_data (keyfile, keyfile_str, strlen (keyfile_str), G_KEY_FILE_NONE, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); con = nm_keyfile_read (keyfile, base_dir, NULL, NULL, &error); - g_assert_no_error (error); - g_assert (NM_IS_CONNECTION (con)); - - g_key_file_unref (keyfile); + nmtst_assert_success (NM_IS_CONNECTION (con), error); nm_keyfile_read_ensure_id (con, filename); nm_keyfile_read_ensure_uuid (con, full_filename); @@ -2151,4 +2147,50 @@ typedef enum { #endif /* __NM_CONNECTION_H__ */ +/*****************************************************************************/ + +static inline void +nmtst_keyfile_assert_data (GKeyFile *kf, const char *data, gssize data_len) +{ + gs_unref_keyfile GKeyFile *kf2 = NULL; + gs_free_error GError *error = NULL; + gs_free char *d1 = NULL; + gs_free char *d2 = NULL; + gboolean success; + gsize d1_len; + gsize d2_len; + + g_assert (kf); + g_assert (data || data_len == 0); + g_assert (data_len >= -1); + + d1 = g_key_file_to_data (kf, &d1_len, &error); + nmtst_assert_success (d1, error); + + if (data_len == -1) { + g_assert_cmpint (strlen (d1), ==, d1_len); + data_len = strlen (data); + g_assert_cmpstr (d1, ==, data); + } + + g_assert_cmpmem (d1, d1_len, data, (gsize) data_len); + + /* also check that we can re-generate the same keyfile from the data. */ + + kf2 = g_key_file_new (); + success = g_key_file_load_from_data (kf2, + d1, + d1_len, + G_KEY_FILE_NONE, + &error); + nmtst_assert_success (success, error); + + d2 = g_key_file_to_data (kf2, &d2_len, &error); + nmtst_assert_success (d2, error); + + g_assert_cmpmem (d2, d2_len, d1, d1_len); +} + +/*****************************************************************************/ + #endif /* __NM_TEST_UTILS_H__ */ From 33bf73f2521340626f56ad14b11f52dabdf709e9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 11:17:18 +0100 Subject: [PATCH 05/19] shared: add typed nm_g_object_set_property*() helpers Add helper wrappers around nm_g_object_set_property() that take a native value, construct a GValue of the according type, and call nm_g_object_set_property(). --- shared/nm-utils/nm-shared-utils.c | 126 +++++++++++++++++++++++++++--- shared/nm-utils/nm-shared-utils.h | 58 +++++++++++++- 2 files changed, 171 insertions(+), 13 deletions(-) diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 9b020429d6..6131d3e809 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -1108,7 +1108,7 @@ nm_utils_error_is_notfound (GError *error) */ gboolean nm_g_object_set_property (GObject *object, - const char *property_name, + const char *property_name, const GValue *value, GError **error) { @@ -1181,17 +1181,90 @@ nm_g_object_set_property (GObject *object, return TRUE; } +#define _set_property(object, property_name, gtype, gtype_set, value, error) \ + G_STMT_START { \ + nm_auto_unset_gvalue GValue gvalue = { 0 }; \ + \ + g_value_init (&gvalue, gtype); \ + gtype_set (&gvalue, (value)); \ + return nm_g_object_set_property ((object), (property_name), &gvalue, (error)); \ + } G_STMT_END + +gboolean +nm_g_object_set_property_string (GObject *object, + const char *property_name, + const char *value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_STRING, g_value_set_string, value, error); +} + +gboolean +nm_g_object_set_property_string_static (GObject *object, + const char *property_name, + const char *value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_STRING, g_value_set_static_string, value, error); +} + +gboolean +nm_g_object_set_property_string_take (GObject *object, + const char *property_name, + char *value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_STRING, g_value_take_string, value, error); +} + gboolean nm_g_object_set_property_boolean (GObject *object, - const char *property_name, + const char *property_name, gboolean value, GError **error) { - nm_auto_unset_gvalue GValue gvalue = { 0 }; + _set_property (object, property_name, G_TYPE_BOOLEAN, g_value_set_boolean, !!value, error); +} - g_value_init (&gvalue, G_TYPE_BOOLEAN); - g_value_set_boolean (&gvalue, !!value); - return nm_g_object_set_property (object, property_name, &gvalue, error); +gboolean +nm_g_object_set_property_char (GObject *object, + const char *property_name, + gint8 value, + GError **error) +{ + /* glib says about G_TYPE_CHAR: + * + * The type designated by G_TYPE_CHAR is unconditionally an 8-bit signed integer. + * + * This is always a (signed!) char. */ + _set_property (object, property_name, G_TYPE_CHAR, g_value_set_schar, value, error); +} + +gboolean +nm_g_object_set_property_uchar (GObject *object, + const char *property_name, + guint8 value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_UCHAR, g_value_set_uchar, value, error); +} + +gboolean +nm_g_object_set_property_int (GObject *object, + const char *property_name, + int value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_INT, g_value_set_int, value, error); +} + +gboolean +nm_g_object_set_property_int64 (GObject *object, + const char *property_name, + gint64 value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_INT64, g_value_set_int64, value, error); } gboolean @@ -1200,11 +1273,44 @@ nm_g_object_set_property_uint (GObject *object, guint value, GError **error) { - nm_auto_unset_gvalue GValue gvalue = { 0 }; + _set_property (object, property_name, G_TYPE_UINT, g_value_set_uint, value, error); +} - g_value_init (&gvalue, G_TYPE_UINT); - g_value_set_uint (&gvalue, value); - return nm_g_object_set_property (object, property_name, &gvalue, error); +gboolean +nm_g_object_set_property_uint64 (GObject *object, + const char *property_name, + guint64 value, + GError **error) +{ + _set_property (object, property_name, G_TYPE_UINT64, g_value_set_uint64, value, error); +} + +gboolean +nm_g_object_set_property_flags (GObject *object, + const char *property_name, + GType gtype, + guint value, + GError **error) +{ + nm_assert (({ + nm_auto_unref_gtypeclass GTypeClass *gtypeclass = g_type_class_ref (gtype); + G_IS_FLAGS_CLASS (gtypeclass); + })); + _set_property (object, property_name, gtype, g_value_set_flags, value, error); +} + +gboolean +nm_g_object_set_property_enum (GObject *object, + const char *property_name, + GType gtype, + int value, + GError **error) +{ + nm_assert (({ + nm_auto_unref_gtypeclass GTypeClass *gtypeclass = g_type_class_ref (gtype); + G_IS_ENUM_CLASS (gtypeclass); + })); + _set_property (object, property_name, gtype, g_value_set_enum, value, error); } GParamSpec * diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index eab9f5acfc..e560697b1a 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -697,20 +697,72 @@ nm_utils_error_set_literal (GError **error, int error_code, const char *literal) /*****************************************************************************/ gboolean nm_g_object_set_property (GObject *object, - const char *property_name, + const char *property_name, const GValue *value, GError **error); +gboolean nm_g_object_set_property_string (GObject *object, + const char *property_name, + const char *value, + GError **error); + +gboolean nm_g_object_set_property_string_static (GObject *object, + const char *property_name, + const char *value, + GError **error); + +gboolean nm_g_object_set_property_string_take (GObject *object, + const char *property_name, + char *value, + GError **error); + gboolean nm_g_object_set_property_boolean (GObject *object, - const char *property_name, + const char *property_name, gboolean value, GError **error); +gboolean nm_g_object_set_property_char (GObject *object, + const char *property_name, + gint8 value, + GError **error); + +gboolean nm_g_object_set_property_uchar (GObject *object, + const char *property_name, + guint8 value, + GError **error); + +gboolean nm_g_object_set_property_int (GObject *object, + const char *property_name, + int value, + GError **error); + +gboolean nm_g_object_set_property_int64 (GObject *object, + const char *property_name, + gint64 value, + GError **error); + gboolean nm_g_object_set_property_uint (GObject *object, - const char *property_name, + const char *property_name, guint value, GError **error); +gboolean nm_g_object_set_property_uint64 (GObject *object, + const char *property_name, + guint64 value, + GError **error); + +gboolean nm_g_object_set_property_flags (GObject *object, + const char *property_name, + GType gtype, + guint value, + GError **error); + +gboolean nm_g_object_set_property_enum (GObject *object, + const char *property_name, + GType gtype, + int value, + GError **error); + GParamSpec *nm_g_object_class_find_property_from_gtype (GType gtype, const char *property_name); From 1b361aaea9e8d024de73330235cc1d0ae515b112 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jan 2019 15:24:35 +0100 Subject: [PATCH 06/19] Revert "libnm-core: don't serialize synthetic properties in nm_setting_to_string()" We shall not shortcut the synth function. If the synth function is unhappy about a missing NMConnection argument, then that needs to be fixed. So, revert 395c385b9 and fix the issue in nm_setting_wireless_get_security() differently. I presume that is the only place that caused problems, since the history of the patch does not clealy show what the problem was. This reverts commit 395c385b9b3dfd469c50144ab7a53b2adf72c2c8. --- libnm-core/nm-connection.h | 2 -- libnm-core/nm-core-internal.h | 7 ------- libnm-core/nm-setting-wireless.c | 3 +++ libnm-core/nm-setting.c | 14 ++++---------- 4 files changed, 7 insertions(+), 19 deletions(-) diff --git a/libnm-core/nm-connection.h b/libnm-core/nm-connection.h index fb1f901cc6..8d64a4cebc 100644 --- a/libnm-core/nm-connection.h +++ b/libnm-core/nm-connection.h @@ -121,8 +121,6 @@ typedef enum { /*< flags >*/ NM_CONNECTION_SERIALIZE_ALL = 0x00000000, NM_CONNECTION_SERIALIZE_NO_SECRETS = 0x00000001, NM_CONNECTION_SERIALIZE_ONLY_SECRETS = 0x00000002, - - /* 0x80000000 is used for a private flag */ } NMConnectionSerializationFlags; GVariant *nm_connection_to_dbus (NMConnection *connection, diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index ff2444673f..056269f36b 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -174,13 +174,6 @@ NMSettingPriority _nm_setting_get_setting_priority (NMSetting *setting); gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value); -/* NM_CONNECTION_SERIALIZE_NO_SYNTH: This flag is passed to _nm_setting_to_dbus() - * by nm_setting_to_string() to let it know that it shouldn't serialize the - * synthetic properties. It wouldn't be able to do so, since the full connection - * is not available, only the setting alone. - */ -#define NM_CONNECTION_SERIALIZE_NO_SYNTH ((NMConnectionSerializationFlags) 0x80000000) - /*****************************************************************************/ GHashTable *_nm_setting_gendata_hash (NMSetting *setting, diff --git a/libnm-core/nm-setting-wireless.c b/libnm-core/nm-setting-wireless.c index 287c27dacb..31eda97fa8 100644 --- a/libnm-core/nm-setting-wireless.c +++ b/libnm-core/nm-setting-wireless.c @@ -953,6 +953,9 @@ nm_setting_wireless_get_security (NMSetting *setting, NMConnection *connection, const char *property_name) { + if (!connection) + return NULL; + if (nm_connection_get_setting_wireless_security (connection)) return g_variant_new_string (NM_SETTING_WIRELESS_SECURITY_SETTING_NAME); else diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 4324f0a4dc..e5e9ab9655 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -685,15 +685,10 @@ _nm_setting_to_dbus (NMSetting *setting, NMConnection *connection, NMConnectionS continue; } - if (property->synth_func) { - if (!(flags & NM_CONNECTION_SERIALIZE_NO_SYNTH)) - dbus_value = property->synth_func (setting, connection, property->name); - else - dbus_value = NULL; - } else { + if (property->synth_func) + dbus_value = property->synth_func (setting, connection, property->name); + else dbus_value = get_property_for_dbus (setting, property, TRUE); - } - if (dbus_value) { /* Allow dbus_value to be either floating or not. */ g_variant_take_ref (dbus_value); @@ -2018,8 +2013,7 @@ nm_setting_to_string (NMSetting *setting) string = g_string_new (nm_setting_get_name (setting)); g_string_append_c (string, '\n'); - variant = _nm_setting_to_dbus (setting, NULL, NM_CONNECTION_SERIALIZE_ALL - | NM_CONNECTION_SERIALIZE_NO_SYNTH); + variant = _nm_setting_to_dbus (setting, NULL, NM_CONNECTION_SERIALIZE_ALL); g_variant_iter_init (&iter, variant); while ((child = g_variant_iter_next_value (&iter))) { From e8bf89a90679274609c7e9b8fcb7b2ef13f89b83 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jan 2019 15:54:18 +0100 Subject: [PATCH 07/19] libnm: pass serialization flags to settings synth_func() We will need access to the serialization flags from within the synth_func(). That will be for WireGuard's peers. Peers are a list of complex, structured elements, and some fields (the peer's preshared-key) are secret and others are not. So when serializing the peers, we need to know whether to include secrets or not. Instead of letting _nm_setting_to_dbus() check the flags, pass them down. While at it, don't pass the property_name argument. Instead, pass the entire meta-data information we have. Most synth functions don't care about the property or the name either way. But we should not pre-filter information that we have at hand. Just pass it to the synth function. If the synth function would be public API, that would be a reason to be careful about what we pass. But it isn't and it only has one caller. So passing it along is fine. Also, do it now when adding the flags argument, as we touch all synth implementations anyway. --- libnm-core/nm-core-internal.h | 6 +++-- libnm-core/nm-setting-ip4-config.c | 43 +++++++++++++++++------------- libnm-core/nm-setting-ip6-config.c | 34 ++++++++++++----------- libnm-core/nm-setting-private.h | 6 +++-- libnm-core/nm-setting-wireless.c | 15 +++++++---- libnm-core/nm-setting.c | 18 ++++++++----- libnm-core/nm-utils-private.h | 6 +++-- libnm-core/nm-utils.c | 15 ++++++++--- 8 files changed, 88 insertions(+), 55 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 056269f36b..ec9b108318 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -582,9 +582,11 @@ typedef struct _NMSettInfoSetting NMSettInfoSetting; typedef GVariant *(*NMSettingPropertyGetFunc) (NMSetting *setting, const char *property); -typedef GVariant *(*NMSettingPropertySynthFunc) (NMSetting *setting, +typedef GVariant *(*NMSettingPropertySynthFunc) (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property); + NMSetting *setting, + NMConnectionSerializationFlags flags); typedef gboolean (*NMSettingPropertySetFunc) (NMSetting *setting, GVariant *connection_dict, const char *property, diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c index 36765484eb..adc8243403 100644 --- a/libnm-core/nm-setting-ip4-config.c +++ b/libnm-core/nm-setting-ip4-config.c @@ -376,9 +376,11 @@ ip4_addresses_set (NMSetting *setting, } static GVariant * -ip4_address_labels_get (NMSetting *setting, +ip4_address_labels_get (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { NMSettingIPConfig *s_ip = NM_SETTING_IP_CONFIG (setting); gboolean have_labels = FALSE; @@ -386,6 +388,9 @@ ip4_address_labels_get (NMSetting *setting, GVariant *ret; int num_addrs, i; + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; + num_addrs = nm_setting_ip_config_get_num_addresses (s_ip); for (i = 0; i < num_addrs; i++) { NMIPAddress *addr = nm_setting_ip_config_get_address (s_ip, i); @@ -414,18 +419,19 @@ ip4_address_labels_get (NMSetting *setting, } static GVariant * -ip4_address_data_get (NMSetting *setting, +ip4_address_data_get (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { - GPtrArray *addrs; - GVariant *ret; + gs_unref_ptrarray GPtrArray *addrs = NULL; + + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; g_object_get (setting, NM_SETTING_IP_CONFIG_ADDRESSES, &addrs, NULL); - ret = nm_utils_ip_addresses_to_variant (addrs); - g_ptr_array_unref (addrs); - - return ret; + return nm_utils_ip_addresses_to_variant (addrs); } static gboolean @@ -486,18 +492,19 @@ ip4_routes_set (NMSetting *setting, } static GVariant * -ip4_route_data_get (NMSetting *setting, +ip4_route_data_get (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { - GPtrArray *routes; - GVariant *ret; + gs_unref_ptrarray GPtrArray *routes = NULL; + + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; g_object_get (setting, NM_SETTING_IP_CONFIG_ROUTES, &routes, NULL); - ret = nm_utils_ip_routes_to_variant (routes); - g_ptr_array_unref (routes); - - return ret; + return nm_utils_ip_routes_to_variant (routes); } static gboolean diff --git a/libnm-core/nm-setting-ip6-config.c b/libnm-core/nm-setting-ip6-config.c index fa65be910a..153df54f2f 100644 --- a/libnm-core/nm-setting-ip6-config.c +++ b/libnm-core/nm-setting-ip6-config.c @@ -374,18 +374,19 @@ ip6_addresses_set (NMSetting *setting, } static GVariant * -ip6_address_data_get (NMSetting *setting, +ip6_address_data_get (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { - GPtrArray *addrs; - GVariant *ret; + gs_unref_ptrarray GPtrArray *addrs = NULL; + + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; g_object_get (setting, NM_SETTING_IP_CONFIG_ADDRESSES, &addrs, NULL); - ret = nm_utils_ip_addresses_to_variant (addrs); - g_ptr_array_unref (addrs); - - return ret; + return nm_utils_ip_addresses_to_variant (addrs); } static gboolean @@ -446,18 +447,19 @@ ip6_routes_set (NMSetting *setting, } static GVariant * -ip6_route_data_get (NMSetting *setting, +ip6_route_data_get (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { - GPtrArray *routes; - GVariant *ret; + gs_unref_ptrarray GPtrArray *routes = NULL; + + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; g_object_get (setting, NM_SETTING_IP_CONFIG_ROUTES, &routes, NULL); - ret = nm_utils_ip_routes_to_variant (routes); - g_ptr_array_unref (routes); - - return ret; + return nm_utils_ip_routes_to_variant (routes); } static gboolean diff --git a/libnm-core/nm-setting-private.h b/libnm-core/nm-setting-private.h index 1e25226ede..c785fbc90f 100644 --- a/libnm-core/nm-setting-private.h +++ b/libnm-core/nm-setting-private.h @@ -82,9 +82,11 @@ gboolean _nm_setting_clear_secrets_with_flags (NMSetting *setting, #define NM_SETTING_PARAM_GENDATA_BACKED (1 << (7 + G_PARAM_USER_SHIFT)) -GVariant *_nm_setting_get_deprecated_virtual_interface_name (NMSetting *setting, +GVariant *_nm_setting_get_deprecated_virtual_interface_name (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property); + NMSetting *setting, + NMConnectionSerializationFlags flags); NMSettingVerifyResult _nm_setting_verify (NMSetting *setting, NMConnection *connection, diff --git a/libnm-core/nm-setting-wireless.c b/libnm-core/nm-setting-wireless.c index 31eda97fa8..02ff534b2f 100644 --- a/libnm-core/nm-setting-wireless.c +++ b/libnm-core/nm-setting-wireless.c @@ -949,17 +949,22 @@ compare_property (NMSetting *setting, /*****************************************************************************/ static GVariant * -nm_setting_wireless_get_security (NMSetting *setting, +nm_setting_wireless_get_security (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property_name) + NMSetting *setting, + NMConnectionSerializationFlags flags) { + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; + if (!connection) return NULL; - if (nm_connection_get_setting_wireless_security (connection)) - return g_variant_new_string (NM_SETTING_WIRELESS_SECURITY_SETTING_NAME); - else + if (!nm_connection_get_setting_wireless_security (connection)) return NULL; + + return g_variant_new_string (NM_SETTING_WIRELESS_SECURITY_SETTING_NAME); } /** diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index e5e9ab9655..5488eea816 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -662,10 +662,13 @@ _nm_setting_to_dbus (NMSetting *setting, NMConnection *connection, NMConnectionS if (!prop_spec) { if (!property->synth_func) continue; - - if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) - continue; } else { + + /* For the moment, properties backed by a GObject property don't + * define a synth function. There is no problem supporting that, + * however, for now just disallow it. */ + nm_assert (!property->synth_func); + if (!(prop_spec->flags & G_PARAM_WRITABLE)) continue; @@ -686,9 +689,10 @@ _nm_setting_to_dbus (NMSetting *setting, NMConnection *connection, NMConnectionS } if (property->synth_func) - dbus_value = property->synth_func (setting, connection, property->name); + dbus_value = property->synth_func (sett_info, i, connection, setting, flags); else dbus_value = get_property_for_dbus (setting, property, TRUE); + if (dbus_value) { /* Allow dbus_value to be either floating or not. */ g_variant_take_ref (dbus_value); @@ -2031,9 +2035,11 @@ nm_setting_to_string (NMSetting *setting) } GVariant * -_nm_setting_get_deprecated_virtual_interface_name (NMSetting *setting, +_nm_setting_get_deprecated_virtual_interface_name (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { NMSettingConnection *s_con; diff --git a/libnm-core/nm-utils-private.h b/libnm-core/nm-utils-private.h index b886730a35..58822cdeff 100644 --- a/libnm-core/nm-utils-private.h +++ b/libnm-core/nm-utils-private.h @@ -56,9 +56,11 @@ gboolean _nm_utils_hwaddr_cloned_not_set (NMSetting *setting, const char *property, NMSettingParseFlags parse_flags, GError **error); -GVariant * _nm_utils_hwaddr_cloned_data_synth (NMSetting *setting, +GVariant * _nm_utils_hwaddr_cloned_data_synth (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property); + NMSetting *setting, + NMConnectionSerializationFlags flags); gboolean _nm_utils_hwaddr_cloned_data_set (NMSetting *setting, GVariant *connection_dict, const char *property, diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 69657c9dce..4709971281 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4234,13 +4234,18 @@ _nm_utils_hwaddr_cloned_not_set (NMSetting *setting, } GVariant * -_nm_utils_hwaddr_cloned_data_synth (NMSetting *setting, +_nm_utils_hwaddr_cloned_data_synth (const NMSettInfoSetting *sett_info, + guint property_idx, NMConnection *connection, - const char *property) + NMSetting *setting, + NMConnectionSerializationFlags flags) { gs_free char *addr = NULL; - nm_assert (nm_streq0 (property, "assigned-mac-address")); + if (flags & NM_CONNECTION_SERIALIZE_ONLY_SECRETS) + return NULL; + + nm_assert (nm_streq0 (sett_info->property_infos[property_idx].name, "assigned-mac-address")); g_object_get (setting, "cloned-mac-address", @@ -4261,7 +4266,9 @@ _nm_utils_hwaddr_cloned_data_synth (NMSetting *setting, * To preserve that behavior, serialize "" as NULL. */ - return addr && addr[0] ? g_variant_new_string (addr) : NULL; + return addr && addr[0] + ? g_variant_new_take_string (g_steal_pointer (&addr)) + : NULL; } gboolean From 70fab489db960105aeb0c1c1b64f55485fe4d718 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 30 Dec 2018 15:05:14 +0100 Subject: [PATCH 08/19] keyfile: add nm_keyfile_plugin_kf_get_int64() helper --- libnm-core/nm-keyfile-utils.c | 32 ++++++++++++++++++++++++++++++++ libnm-core/nm-keyfile-utils.h | 9 +++++++++ po/POTFILES.in | 1 + 3 files changed, 42 insertions(+) diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c index ff12fa1063..bf841cc0e5 100644 --- a/libnm-core/nm-keyfile-utils.c +++ b/libnm-core/nm-keyfile-utils.c @@ -184,6 +184,38 @@ DEFINE_KF_WRAPPER(uint64, guint64, guint64); DEFINE_KF_WRAPPER(boolean, gboolean, gboolean); DEFINE_KF_WRAPPER(value, char*, const char*); +gint64 +nm_keyfile_plugin_kf_get_int64 (GKeyFile *kf, + const char *group, + const char *key, + guint base, + gint64 min, + gint64 max, + gint64 fallback, + GError **error) +{ + gs_free char *s = NULL; + int errsv; + gint64 v; + + s = nm_keyfile_plugin_kf_get_value (kf, group, key, error); + if (!s) { + errno = ENODATA; + return fallback; + } + + v = _nm_utils_ascii_str_to_int64 (s, base, min, max, fallback); + errsv = errno; + if ( errsv != 0 + && error) { + g_set_error (error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value is not an integer in range [%lld, %lld]"), + (long long) min, (long long) max); + errno = errsv; + } + return v; +} + char ** nm_keyfile_plugin_kf_get_keys (GKeyFile *kf, const char *group, diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h index 18d5dce831..aa647cf798 100644 --- a/libnm-core/nm-keyfile-utils.h +++ b/libnm-core/nm-keyfile-utils.h @@ -73,6 +73,15 @@ DEFINE_KF_WRAPPER_PROTO(boolean, gboolean, gboolean) DEFINE_KF_WRAPPER_PROTO(value, char*, const char*) /* Misc */ +gint64 nm_keyfile_plugin_kf_get_int64 (GKeyFile *kf, + const char *group, + const char *key, + guint base, + gint64 min, + gint64 max, + gint64 fallback, + GError **error); + char ** nm_keyfile_plugin_kf_get_keys (GKeyFile *kf, const char *group, gsize *out_length, diff --git a/po/POTFILES.in b/po/POTFILES.in index 942b066131..33192b1dd0 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -56,6 +56,7 @@ libnm-core/nm-crypto-gnutls.c libnm-core/nm-crypto-nss.c libnm-core/nm-connection.c libnm-core/nm-dbus-utils.c +libnm-core/nm-keyfile-utils.c libnm-core/nm-keyfile.c libnm-core/nm-setting-6lowpan.c libnm-core/nm-setting-8021x.c From 7cd6c175e401df4098687b1a8d4f5f1dc154bfe8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jan 2019 12:39:05 +0100 Subject: [PATCH 09/19] keyfile: add _secret_flags_persist_secret() function for skipping secrets based on flags --- libnm-core/nm-keyfile.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index cec09c9f38..b376f842f0 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -96,6 +96,15 @@ _handle_warn (KeyfileReaderInfo *info, _info->error == NULL; \ }) +/*****************************************************************************/ + +static gboolean +_secret_flags_persist_secret (NMSettingSecretFlags flags) +{ + return flags == NM_SETTING_SECRET_FLAG_NONE; +} + +/*****************************************************************************/ /* Some setting properties also contain setting names, such as * NMSettingConnection's 'type' property (which specifies the base type of the * connection, e.g. ethernet or wifi) or 'slave-type' (specifies type of slave @@ -1863,8 +1872,8 @@ write_hash_of_string (GKeyFile *file, keys = nm_utils_strdict_get_keys (hash, TRUE, &l); for (i = 0; i < l; i++) { + gs_free char *to_free = NULL; const char *property, *data; - gboolean write_item = TRUE; property = keys[i]; @@ -1876,18 +1885,14 @@ write_hash_of_string (GKeyFile *file, NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; nm_setting_get_secret_flags (setting, property, &secret_flags, NULL); - if (secret_flags != NM_SETTING_SECRET_FLAG_NONE) - write_item = FALSE; + if (!_secret_flags_persist_secret (secret_flags)) + continue; } - if (write_item) { - gs_free char *to_free = NULL; - - data = g_hash_table_lookup (hash, property); - nm_keyfile_plugin_kf_set_string (file, group_name, - nm_keyfile_key_encode (property, &to_free), - data); - } + data = g_hash_table_lookup (hash, property); + nm_keyfile_plugin_kf_set_string (file, group_name, + nm_keyfile_key_encode (property, &to_free), + data); } } @@ -3023,8 +3028,8 @@ write_setting_value (NMSetting *setting, NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL)) - g_assert_not_reached (); - if (secret_flags != NM_SETTING_SECRET_FLAG_NONE) + g_return_if_reached (); + if (!_secret_flags_persist_secret (secret_flags)) return; } From 6d9bea09a73047cf518169f65e18052c81104947 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 13:24:06 +0100 Subject: [PATCH 10/19] libnm/tests: add tests for converting profiles to keyfile and back --- libnm-core/tests/test-setting.c | 238 ++++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+) diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 79c8257610..f9b4fc33d4 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -45,6 +45,77 @@ /*****************************************************************************/ +/* converts @dict to a connection. In this case, @dict must be good, without warnings, so that + * NM_SETTING_PARSE_FLAGS_STRICT and NM_SETTING_PARSE_FLAGS_BEST_EFFORT yield the exact same results. */ +static NMConnection * +_connection_new_from_dbus_strict (GVariant *dict, + gboolean normalize) +{ + gs_unref_object NMConnection *con_x_0 = NULL; + gs_unref_object NMConnection *con_x_s = NULL; + gs_unref_object NMConnection *con_x_e = NULL; + gs_unref_object NMConnection *con_n_0 = NULL; + gs_unref_object NMConnection *con_n_s = NULL; + gs_unref_object NMConnection *con_n_e = NULL; + gs_free_error GError *error = NULL; + guint i; + + g_assert (g_variant_is_of_type (dict, NM_VARIANT_TYPE_CONNECTION)); + + con_x_0 = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_NONE, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_x_0), error); + + con_x_s = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_STRICT, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_x_s), error); + + con_x_e = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_x_e), error); + + con_n_0 = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_NORMALIZE, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_n_0), error); + + con_n_s = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_STRICT | NM_SETTING_PARSE_FLAGS_NORMALIZE, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_n_s), error); + + con_n_e = _nm_simple_connection_new_from_dbus (dict, NM_SETTING_PARSE_FLAGS_BEST_EFFORT | NM_SETTING_PARSE_FLAGS_NORMALIZE, &error); + nmtst_assert_success (NM_IS_CONNECTION (con_n_e), error); + + nmtst_assert_connection_verifies (con_x_0); + nmtst_assert_connection_verifies (con_x_e); + nmtst_assert_connection_verifies (con_x_s); + + nmtst_assert_connection_verifies_without_normalization (con_n_0); + nmtst_assert_connection_verifies_without_normalization (con_n_e); + nmtst_assert_connection_verifies_without_normalization (con_n_s); + + /* randomly compare some pairs that we created. They must all be equal, + * after accounting for normalization. */ + for (i = 0; i < 10; i++) { + NMConnection *cons[] = { con_x_0, con_x_s, con_x_e, con_n_0, con_n_s, con_n_e }; + guint idx_a = (nmtst_get_rand_int () % G_N_ELEMENTS (cons)); + guint idx_b = (nmtst_get_rand_int () % G_N_ELEMENTS (cons)); + gboolean normalize_a, normalize_b; + + if (idx_a <= 2 && idx_b <= 2) { + normalize_a = nmtst_get_rand_bool (); + normalize_b = normalize_a; + } else if (idx_a > 2 && idx_b > 2) { + normalize_a = nmtst_get_rand_bool (); + normalize_b = nmtst_get_rand_bool (); + } else { + normalize_a = (idx_a <= 2) ? TRUE : nmtst_get_rand_bool (); + normalize_b = (idx_b <= 2) ? TRUE : nmtst_get_rand_bool (); + } + nmtst_assert_connection_equals (cons[idx_a], normalize_a, cons[idx_b], normalize_b); + } + + return (normalize) + ? g_steal_pointer (&con_x_0) + : g_steal_pointer (&con_n_0); +} + +/*****************************************************************************/ + static void compare_blob_data (const char *test, const char *key_path, @@ -1943,6 +2014,171 @@ test_tc_config_dbus (void) /*****************************************************************************/ +static void +test_roundtrip_conversion (gconstpointer test_data) +{ + const int MODE = GPOINTER_TO_INT (test_data); + const char *ID= nm_sprintf_bufa (100, "roundtip-conversion-%d", MODE); + const char *UUID= "63376701-b61e-4318-bf7e-664a1c1eeaab"; + const char *INTERFACE_NAME = nm_sprintf_bufa (100, "ifname%d", MODE); + guint32 ETH_MTU = nmtst_rand_select ((guint32) 0u, + nmtst_get_rand_int ()); + gs_unref_ptrarray GPtrArray *kf_data_arr = g_ptr_array_new_with_free_func (g_free); + const NMConnectionSerializationFlags dbus_serialization_flags[] = { + NM_CONNECTION_SERIALIZE_ALL, + NM_CONNECTION_SERIALIZE_NO_SECRETS, + NM_CONNECTION_SERIALIZE_ONLY_SECRETS, + }; + guint dbus_serialization_flags_idx; + gs_unref_object NMConnection *con = NULL; + gs_free_error GError *error = NULL; + guint kf_data_idx; + NMSettingConnection *s_con = NULL; + NMSettingWired *s_eth = NULL; + + switch (MODE) { + case 0: + con = nmtst_create_minimal_connection (ID, UUID, NM_SETTING_WIRED_SETTING_NAME, &s_con); + g_object_set (s_con, + NM_SETTING_CONNECTION_INTERFACE_NAME, + INTERFACE_NAME, + NULL); + nmtst_connection_normalize (con); + + s_eth = NM_SETTING_WIRED (nm_connection_get_setting (con, NM_TYPE_SETTING_WIRED)); + g_assert (NM_IS_SETTING_WIRED (s_eth)); + + g_object_set (s_eth, + NM_SETTING_WIRED_MTU, + ETH_MTU, + NULL); + + g_ptr_array_add (kf_data_arr, + g_strdup_printf ("[connection]\n" + "id=%s\n" + "uuid=%s\n" + "type=ethernet\n" + "interface-name=%s\n" + "permissions=\n" + "\n" + "[ethernet]\n" + "mac-address-blacklist=\n" + "%s" /* mtu */ + "\n" + "[ipv4]\n" + "dns-search=\n" + "method=auto\n" + "\n" + "[ipv6]\n" + "addr-gen-mode=stable-privacy\n" + "dns-search=\n" + "method=auto\n" + "", + ID, + UUID, + INTERFACE_NAME, + (ETH_MTU != 0) + ? nm_sprintf_bufa (100, "mtu=%d\n", (int) ETH_MTU) + : "")); + + g_ptr_array_add (kf_data_arr, + g_strdup_printf ("[connection]\n" + "id=%s\n" + "uuid=%s\n" + "type=ethernet\n" + "interface-name=%s\n" + "permissions=\n" + "\n" + "[ethernet]\n" + "mac-address-blacklist=\n" + "%s" /* mtu */ + "\n" + "[ipv4]\n" + "dns-search=\n" + "method=auto\n" + "\n" + "[ipv6]\n" + "addr-gen-mode=stable-privacy\n" + "dns-search=\n" + "method=auto\n" + "", + ID, + UUID, + INTERFACE_NAME, + (ETH_MTU != 0) + ? nm_sprintf_bufa (100, "mtu=%u\n", ETH_MTU) + : "")); + + break; + + default: + g_assert_not_reached (); + } + + /* the first kf_data_arr entry is special: it is the exact result of what we expect + * when converting @con to keyfile. Write @con to keyfile and compare the expected result + * literally. */ + { + gs_unref_keyfile GKeyFile *kf = NULL; + + kf = nm_keyfile_write (con, NULL, NULL, &error); + nmtst_assert_success (kf, error); + + /* the first kf_data_arr entry is special: it must be what the writer would + * produce again. */ + nmtst_keyfile_assert_data (kf, kf_data_arr->pdata[0], -1); + } + + /* check that reading any of kf_data_arr yields the same result that we expect. */ + for (kf_data_idx = 0; kf_data_idx < kf_data_arr->len; kf_data_idx++) { + gs_unref_object NMConnection *con2 = NULL; + NMSettingWired *s_eth2 = NULL; + + con2 = nmtst_create_connection_from_keyfile (kf_data_arr->pdata[kf_data_idx], "/no/where/file.nmconnection"); + + switch (MODE) { + case 0: + s_eth2 = NM_SETTING_WIRED (nm_connection_get_setting (con2, NM_TYPE_SETTING_WIRED)); + g_assert (NM_IS_SETTING_WIRED (s_eth2)); + + if ( ETH_MTU > (guint32) G_MAXINT + && kf_data_idx == 1) { + /* values > 2^21 get written as signed integers. When reading this back, + * positive values are ignored. Patch the MTU in s_eth2. */ + g_assert_cmpint (nm_setting_wired_get_mtu (s_eth2), ==, 0); + g_object_set (s_eth2, + NM_SETTING_WIRED_MTU, + ETH_MTU, + NULL); + } + + g_assert_cmpint (nm_setting_wired_get_mtu (s_eth), ==, ETH_MTU); + g_assert_cmpint (nm_setting_wired_get_mtu (s_eth2), ==, ETH_MTU); + break; + } + + nmtst_assert_connection_equals (con, nmtst_get_rand_bool (), con2, nmtst_get_rand_bool ()); + } + + for (dbus_serialization_flags_idx = 0; dbus_serialization_flags_idx < G_N_ELEMENTS (dbus_serialization_flags); dbus_serialization_flags_idx++) { + NMConnectionSerializationFlags flag = dbus_serialization_flags[dbus_serialization_flags_idx]; + gs_unref_variant GVariant *con_var = NULL; + gs_unref_object NMConnection *con2 = NULL; + + con_var = nm_connection_to_dbus (con, flag); + g_assert (g_variant_is_of_type (con_var, NM_VARIANT_TYPE_CONNECTION)); + g_assert (g_variant_is_floating (con_var)); + g_variant_ref_sink (con_var); + + if (flag == NM_CONNECTION_SERIALIZE_ALL) { + con2 = _connection_new_from_dbus_strict (con_var, TRUE); + nmtst_assert_connection_equals (con, nmtst_get_rand_bool (), con2, nmtst_get_rand_bool ()); + } + } +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -2019,5 +2255,7 @@ main (int argc, char **argv) g_test_add_func ("/libnm/settings/team-port/sycn_from_config_full", test_team_port_full_config); #endif + g_test_add_data_func ("/libnm/settings/roundtrip-conversion/general/0", GINT_TO_POINTER (0), test_roundtrip_conversion); + return g_test_run (); } From 75e42847813410e52c0206e2bcb4f203a3486f06 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Jan 2019 09:26:54 +0100 Subject: [PATCH 11/19] keyfile: rework handling of GObject properties from keyfile - previously, writer would use nm_keyfile_plugin_kf_set_integer() for G_TYPE_UINT types. That means, values larger than G_MAXINT would be stored as negative values. On the other hand, the reader would always reject negative values. Fix that, by parsing the integer ourself. Note that we still reject the old (negative) values and there is no compatibility for accepting such values. They were not accepted by reader in the past and so they are still rejected. This affects for example ethernet.mtu setting (arguably, the MTU is usually set to small values where the issue was not apparent). This is also covered by a test. - no longer use nm_keyfile_plugin_kf_set_integer(). nm_keyfile_plugin_kf_set_integer() calls g_key_file_get_integer(), which uses g_key_file_parse_integer_as_value(). That one has the odd behavior of accepting "" as valid. Note how that differs from g_key_file_parse_value_as_double() which rejects trailing data. Implement the parsing ourself. There are some changes here: - g_key_file_parse_value_as_integer() uses strtol() with base 10. We no longer require a certain the base, so '0x' hex values are allowed now as well. - bogus suffixes are now rejected but were accepted by g_key_file_parse_value_as_integer(). We however still accept leading and trailing whitespace, as before. - use nm_g_object_set_property*(). g_object_set() asserts that the value is in range. We cannot pass invalid values without checking that they are valid. - emit warnings when values cannot be parsed. Previously they would have been silently ignored or fail an assertion during g_object_set(). - don't use "helpers" like nm_keyfile_plugin_kf_set_uint64(). These merely call GKeyFile's setters (taking care of aliases). The setters of GKeyFile don't do anything miraculously, they merely call g_key_file_set_value() with the string that one would expect. Convert the numbers/boolean ourselfs. For one, we don't require a heap allocation to convert a number to string. Also, there is no point in leaving this GKeyFile API, because even if GKeyFile day would change, we still must continue to support the present format, as that is what users have on disk. So, even if a new way would be implemented by GKeyFile, the current way must forever be accepted too. Hence, we don't need this abstraction. --- libnm-core/nm-keyfile-utils.c | 2 - libnm-core/nm-keyfile-utils.h | 2 - libnm-core/nm-keyfile.c | 329 ++++++++++++++++++++------------ libnm-core/tests/test-setting.c | 13 +- 4 files changed, 217 insertions(+), 129 deletions(-) diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c index bf841cc0e5..9543ebde6e 100644 --- a/libnm-core/nm-keyfile-utils.c +++ b/libnm-core/nm-keyfile-utils.c @@ -179,8 +179,6 @@ nm_keyfile_plugin_kf_set_##stype (GKeyFile *kf, \ } DEFINE_KF_WRAPPER(string, char*, const char*); -DEFINE_KF_WRAPPER(integer, int, int); -DEFINE_KF_WRAPPER(uint64, guint64, guint64); DEFINE_KF_WRAPPER(boolean, gboolean, gboolean); DEFINE_KF_WRAPPER(value, char*, const char*); diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h index aa647cf798..0467230e49 100644 --- a/libnm-core/nm-keyfile-utils.h +++ b/libnm-core/nm-keyfile-utils.h @@ -67,8 +67,6 @@ void nm_keyfile_plugin_kf_set_##stype (GKeyFile *kf, \ const char *key, \ set_ctype value); DEFINE_KF_WRAPPER_PROTO(string, char*, const char*) -DEFINE_KF_WRAPPER_PROTO(integer, int, int) -DEFINE_KF_WRAPPER_PROTO(uint64, guint64, guint64) DEFINE_KF_WRAPPER_PROTO(boolean, gboolean, gboolean) DEFINE_KF_WRAPPER_PROTO(value, char*, const char*) diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index b376f842f0..7617da44e9 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -1384,55 +1384,119 @@ cert_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) g_object_set (setting, key, bytes, NULL); } +static int +_parity_from_char (int ch) +{ +#if NM_MORE_ASSERTS > 5 + { + static char check = 0; + + if (check == 0) { + nm_auto_unref_gtypeclass GEnumClass *klass = g_type_class_ref (NM_TYPE_SETTING_SERIAL_PARITY); + guint i; + + check = 1; + + /* In older versions, parity was G_TYPE_CHAR/gint8, and the character + * value was stored as integer. + * For example parity=69 equals parity=E, meaning NM_SETTING_SERIAL_PARITY_EVEN. + * + * That means, certain values are reserved. Assert that these numbers + * are not reused when we extend NMSettingSerialParity enum. + * Actually, since NM_SETTING_SERIAL_PARITY is g_param_spec_enum(), + * we anyway cannot extend the enum without breaking API... + * + * [1] commit "a91e60902e libnm-core: make NMSettingSerial:parity an enum" + * [2] https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a91e60902eabae1de93d61323dae6ac894b5d40f + */ + g_assert (G_IS_ENUM_CLASS (klass)); + for (i = 0; i < klass->n_values; i++) { + const GEnumValue *v = &klass->values[i]; + int num = v->value; + + g_assert (_parity_from_char (num) == -1); + g_assert (!NM_IN_SET (num, 'e', 'E', 'o', 'O', 'n', 'N')); + } + } + } +#endif + + switch (ch) { + case 'E': + case 'e': + return NM_SETTING_SERIAL_PARITY_EVEN; + case 'O': + case 'o': + return NM_SETTING_SERIAL_PARITY_ODD; + case 'N': + case 'n': + return NM_SETTING_SERIAL_PARITY_NONE; + } + + return -1; +} + static void parity_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key) { const char *setting_name = nm_setting_get_name (setting); - NMSettingSerialParity parity; - int int_val; - gs_free char *str_val = NULL; + gs_free_error GError *err = NULL; + int parity; + gs_free char *tmp_str = NULL; + gint64 i64; /* Keyfile traditionally stored this as the ASCII value for 'E', 'o', or 'n'. * We now accept either that or the (case-insensitive) character itself (but * still always write it the old way, for backward compatibility). */ - int_val = nm_keyfile_plugin_kf_get_integer (info->keyfile, setting_name, key, NULL); - if (!int_val) { - str_val = nm_keyfile_plugin_kf_get_string (info->keyfile, setting_name, key, NULL); - if (str_val) { - if (str_val[0] && !str_val[1]) - int_val = str_val[0]; - else { - /* This will hit the warning below */ - int_val = 'X'; - } + tmp_str = nm_keyfile_plugin_kf_get_value (info->keyfile, setting_name, key, &err); + if (err) + goto out_err; + + if ( tmp_str + && tmp_str[0] != '\0' + && tmp_str[1] == '\0') { + /* the ASCII characters like 'E' are taken directly... */ + parity = _parity_from_char (tmp_str[0]); + if (parity >= 0) + goto parity_good; + } + + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MININT64); + if ( i64 != G_MININT64 + && errno == 0) { + + if ((parity = _parity_from_char (i64)) >= 0) { + /* another oddity: the string is a valid number. However, if the numeric values + * is one of the supported ASCII codes, accept it (like 69 for 'E'). + */ + goto parity_good; } + + /* Finally, take the numeric value as is. */ + parity = i64; + goto parity_good; } - if (!int_val) - return; + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid parity value '%s'"), + tmp_str ?: ""); + return; - switch (int_val) { - case 'E': - case 'e': - parity = NM_SETTING_SERIAL_PARITY_EVEN; - break; - case 'O': - case 'o': - parity = NM_SETTING_SERIAL_PARITY_ODD; - break; - case 'N': - case 'n': - parity = NM_SETTING_SERIAL_PARITY_NONE; - break; - default: - handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid parity value '%s'"), - str_val ?: ""); +parity_good: + nm_g_object_set_property_enum (G_OBJECT (setting), key, NM_TYPE_SETTING_SERIAL_PARITY, parity, &err); + +out_err: + if (!err) + return; + if ( err->domain == G_KEY_FILE_ERROR + && NM_IN_SET (err->code, G_KEY_FILE_ERROR_GROUP_NOT_FOUND, + G_KEY_FILE_ERROR_KEY_NOT_FOUND)) { + /* ignore such errors. The key is not present. */ return; } - - g_object_set (setting, key, parity, NULL); + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), err->message); } static void @@ -2531,11 +2595,13 @@ read_one_setting_value (NMSetting *setting, { KeyfileReaderInfo *info = user_data; GKeyFile *keyfile = info->keyfile; - const char *setting_name; - int errsv; - GType type; gs_free_error GError *err = NULL; const ParseInfoProperty *pip; + gs_free char *tmp_str = NULL; + const char *setting_name; + GType type; + guint64 u64; + gint64 i64; if (info->error) return; @@ -2581,62 +2647,71 @@ read_one_setting_value (NMSetting *setting, if (type == G_TYPE_STRING) { gs_free char *str_val = NULL; - str_val = nm_keyfile_plugin_kf_get_string (keyfile, setting_name, key, NULL); - g_object_set (setting, key, str_val, NULL); + str_val = nm_keyfile_plugin_kf_get_string (keyfile, setting_name, key, &err); + if (!err) + nm_g_object_set_property_string_take (G_OBJECT (setting), key, g_steal_pointer (&str_val), &err); } else if (type == G_TYPE_UINT) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - if (int_val < 0) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid negative value (%i)"), - int_val)) - return; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_uint (G_OBJECT (setting), key, u64, &err); } - g_object_set (setting, key, int_val, NULL); } else if (type == G_TYPE_INT) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - g_object_set (setting, key, int_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MININT64); + if ( i64 == G_MININT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_int (G_OBJECT (setting), key, i64, &err); + } } else if (type == G_TYPE_BOOLEAN) { gboolean bool_val; - bool_val = nm_keyfile_plugin_kf_get_boolean (keyfile, setting_name, key, NULL); - g_object_set (setting, key, bool_val, NULL); + bool_val = nm_keyfile_plugin_kf_get_boolean (keyfile, setting_name, key, &err); + if (!err) + nm_g_object_set_property_boolean (G_OBJECT (setting), key, bool_val, &err); } else if (type == G_TYPE_CHAR) { - int int_val; - - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, NULL); - if (int_val < G_MININT8 || int_val > G_MAXINT8) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid char value (%i)"), - int_val)) - return; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + /* As documented by glib, G_TYPE_CHAR is really a (signed!) gint8. */ + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT8, G_MAXINT8, G_MININT64); + if ( i64 == G_MININT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_char (G_OBJECT (setting), key, i64, &err); } - - g_object_set (setting, key, int_val, NULL); } else if (type == G_TYPE_UINT64) { - gs_free char *tmp_str = NULL; - guint64 uint_val; - - tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, NULL); - uint_val = g_ascii_strtoull (tmp_str, NULL, 10); - g_object_set (setting, key, uint_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT64, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_uint64 (G_OBJECT (setting), key, u64, &err); + } } else if (type == G_TYPE_INT64) { - gs_free char *tmp_str = NULL; - gint64 int_val; - - tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, NULL); - int_val = _nm_utils_ascii_str_to_int64 (tmp_str, 10, G_MININT64, G_MAXINT64, 0); - errsv = errno; - if (errsv) { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("invalid int64 value (%s)"), - tmp_str)) - return; - } else - g_object_set (setting, key, int_val, NULL); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT64, G_MAXINT64, G_MAXINT64); + if ( i64 == G_MAXINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_int64 (G_OBJECT (setting), key, i64, &err); + } } else if (type == G_TYPE_BYTES) { gs_free int *tmp = NULL; GByteArray *array; @@ -2679,31 +2754,39 @@ read_one_setting_value (NMSetting *setting, } else if (type == G_TYPE_ARRAY) { read_array_of_uint (keyfile, setting, key); } else if (G_VALUE_HOLDS_FLAGS (value)) { - guint64 uint_val; - - /* Flags are guint but GKeyFile has no uint reader, just uint64 */ - uint_val = nm_keyfile_plugin_kf_get_uint64 (keyfile, setting_name, key, &err); + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); if (!err) { - if (uint_val <= G_MAXUINT) - g_object_set (setting, key, (guint) uint_val, NULL); - else { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("too large FLAGS property '%s' (%llu)"), - G_VALUE_TYPE_NAME (value), (unsigned long long) uint_val)) - return; - } + u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT, G_MAXUINT64); + if ( u64 == G_MAXUINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_flags (G_OBJECT (setting), key, type, u64, &err); } } else if (G_VALUE_HOLDS_ENUM (value)) { - int int_val; + tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); + if (!err) { + i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MAXINT64); + if ( i64 == G_MAXINT64 + && errno != 0) { + g_set_error_literal (&err, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_INVALID_VALUE, + _("value cannot be interpreted as integer")); + } else + nm_g_object_set_property_enum (G_OBJECT (setting), key, type, i64, &err); + } + } else + g_return_if_reached (); - int_val = nm_keyfile_plugin_kf_get_integer (keyfile, setting_name, key, &err); - if (!err) - g_object_set (setting, key, (int) int_val, NULL); - } else { - if (!handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, - _("unhandled setting property type '%s'"), - G_VALUE_TYPE_NAME (value))) - return; + if (err) { + if ( err->domain == G_KEY_FILE_ERROR + && NM_IN_SET (err->code, G_KEY_FILE_ERROR_GROUP_NOT_FOUND, + G_KEY_FILE_ERROR_KEY_NOT_FOUND)) { + /* ignore such errors. The key is not present. */ + } else { + handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_WARN, + _("invalid setting: %s"), err->message); + } } } @@ -2990,6 +3073,7 @@ write_setting_value (NMSetting *setting, GType type; const ParseInfoProperty *pip; GParamSpec *pspec; + char numstr[64]; if (info->error) return; @@ -3051,24 +3135,26 @@ write_setting_value (NMSetting *setting, str = g_value_get_string (value); if (str) nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, str); - } else if (type == G_TYPE_UINT) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_uint (value)); - else if (type == G_TYPE_INT) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, g_value_get_int (value)); - else if (type == G_TYPE_UINT64) { - char numstr[30]; - + } else if (type == G_TYPE_UINT) { + nm_sprintf_buf (numstr, "%u", g_value_get_uint (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (type == G_TYPE_INT) { + nm_sprintf_buf (numstr, "%d", g_value_get_int (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (type == G_TYPE_UINT64) { nm_sprintf_buf (numstr, "%" G_GUINT64_FORMAT, g_value_get_uint64 (value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_INT64) { - char numstr[30]; - nm_sprintf_buf (numstr, "%" G_GINT64_FORMAT, g_value_get_int64 (value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BOOLEAN) { - nm_keyfile_plugin_kf_set_boolean (info->keyfile, setting_name, key, g_value_get_boolean (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, + g_value_get_boolean (value) + ? "true" + : "false"); } else if (type == G_TYPE_CHAR) { - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_schar (value)); + nm_sprintf_buf (numstr, "%d", (int) g_value_get_schar (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BYTES) { GBytes *bytes; const guint8 *data; @@ -3089,12 +3175,13 @@ write_setting_value (NMSetting *setting, } else if (type == G_TYPE_ARRAY) { write_array_of_uint (info->keyfile, setting, key, value); } else if (G_VALUE_HOLDS_FLAGS (value)) { - /* Flags are guint but GKeyFile has no uint reader, just uint64 */ - nm_keyfile_plugin_kf_set_uint64 (info->keyfile, setting_name, key, (guint64) g_value_get_flags (value)); - } else if (G_VALUE_HOLDS_ENUM (value)) - nm_keyfile_plugin_kf_set_integer (info->keyfile, setting_name, key, (int) g_value_get_enum (value)); - else - g_warn_if_reached (); + nm_sprintf_buf (numstr, "%u", g_value_get_flags (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else if (G_VALUE_HOLDS_ENUM (value)) { + nm_sprintf_buf (numstr, "%d", g_value_get_enum (value)); + nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); + } else + g_return_if_reached (); } GKeyFile * diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index f9b4fc33d4..66842db92d 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -2078,7 +2078,7 @@ test_roundtrip_conversion (gconstpointer test_data) UUID, INTERFACE_NAME, (ETH_MTU != 0) - ? nm_sprintf_bufa (100, "mtu=%d\n", (int) ETH_MTU) + ? nm_sprintf_bufa (100, "mtu=%u\n", ETH_MTU) : "")); g_ptr_array_add (kf_data_arr, @@ -2106,7 +2106,7 @@ test_roundtrip_conversion (gconstpointer test_data) UUID, INTERFACE_NAME, (ETH_MTU != 0) - ? nm_sprintf_bufa (100, "mtu=%u\n", ETH_MTU) + ? nm_sprintf_bufa (100, "mtu=%d\n", (int) ETH_MTU) : "")); break; @@ -2143,8 +2143,13 @@ test_roundtrip_conversion (gconstpointer test_data) if ( ETH_MTU > (guint32) G_MAXINT && kf_data_idx == 1) { - /* values > 2^21 get written as signed integers. When reading this back, - * positive values are ignored. Patch the MTU in s_eth2. */ + /* older versions wrote values > 2^21 as signed integers, but the reader would + * always reject such negative values for G_TYPE_UINT. + * + * The test case kf_data_idx #1 still writes the values in the old style. + * The behavior was fixed, but such values are still rejected as invalid. + * + * Patch the setting so that the comparison below succeeds are usual. */ g_assert_cmpint (nm_setting_wired_get_mtu (s_eth2), ==, 0); g_object_set (s_eth2, NM_SETTING_WIRED_MTU, From 88da1375ef3a4cad19c0a21055c66f4a1303cf46 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 10:49:50 +0100 Subject: [PATCH 12/19] libnm: use property metadata in nm-settings.c instead of GObject property list We have a concept of setting and property meta-data that extends plain GObject properties. While most properties are indeed backed by an implemented as a GObject property, some are not. Reuse the object property meta-data instead of fetching the list of properties. Note that there is not much change in behavior, because at all places where this is done, properties which are not backed by a GObject property are skipped for the moment. If nothing else, we save needlessly cloning the property list. Later possibly we may no longer want to do that and add virtual functions that can handle all properties. --- libnm-core/nm-setting.c | 99 +++++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 43 deletions(-) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 5488eea816..d8b8095865 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1207,9 +1207,6 @@ nm_setting_compare (NMSetting *a, NMSettingCompareFlags flags) { const NMSettInfoSetting *sett_info; - GParamSpec **property_specs; - guint n_property_specs; - int same = TRUE; guint i; g_return_val_if_fail (NM_IS_SETTING (a), FALSE); @@ -1232,9 +1229,11 @@ nm_setting_compare (NMSetting *a, } /* And now all properties */ - property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (a), &n_property_specs); - for (i = 0; i < n_property_specs && same; i++) { - GParamSpec *prop_spec = property_specs[i]; + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + + if (!prop_spec) + continue; /* Fuzzy compare ignores secrets and properties defined with the FUZZY_IGNORE flag */ if ( NM_FLAGS_HAS (flags, NM_SETTING_COMPARE_FLAG_FUZZY) @@ -1253,11 +1252,11 @@ nm_setting_compare (NMSetting *a, && NM_FLAGS_HAS (prop_spec->flags, NM_SETTING_PARAM_SECRET)) continue; - same = NM_SETTING_GET_CLASS (a)->compare_property (a, b, prop_spec, flags); + if (!NM_SETTING_GET_CLASS (a)->compare_property (a, b, prop_spec, flags)) + return FALSE; } - g_free (property_specs); - return same; + return TRUE; } static inline gboolean @@ -1446,15 +1445,13 @@ nm_setting_diff (NMSetting *a, } } } else { - gs_free GParamSpec **property_specs = NULL; - guint n_property_specs; - - property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (a), &n_property_specs); - - for (i = 0; i < n_property_specs; i++) { - GParamSpec *prop_spec = property_specs[i]; + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; NMSettingDiffResult r = NM_SETTING_DIFF_RESULT_UNKNOWN; + if (!prop_spec) + continue; + /* Handle compare flags */ if (!should_compare_prop (a, prop_spec->name, flags, prop_spec->flags)) continue; @@ -1581,8 +1578,8 @@ nm_setting_enumerate_values (NMSetting *setting, gpointer user_data) { const NMSettInfoSetting *sett_info; - GParamSpec **property_specs; - guint n_properties; + gs_free GParamSpec **property_specs = NULL; + guint n_property_specs; guint i; GType type; @@ -1593,6 +1590,7 @@ nm_setting_enumerate_values (NMSetting *setting, if (sett_info->detail.gendata_info) { const char *const*names; + guint n_properties; /* the properties of this setting are not real GObject properties. * Hence, this API makes little sense (or does it?). Still, call @@ -1622,15 +1620,26 @@ nm_setting_enumerate_values (NMSetting *setting, return; } - property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (setting), &n_properties); + if (sett_info->property_infos_len == 0) + return; /* sort the properties. This has an effect on the order in which keyfile * prints them. */ - type = G_OBJECT_TYPE (setting); - g_qsort_with_data (property_specs, n_properties, sizeof (gpointer), - (GCompareDataFunc) _enumerate_values_sort, &type); + property_specs = g_new (GParamSpec *, sett_info->property_infos_len); + n_property_specs = 0; + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; - for (i = 0; i < n_properties; i++) { + if (prop_spec) + property_specs[n_property_specs++] = prop_spec; + } + if (n_property_specs > 1) { + type = G_OBJECT_TYPE (setting); + g_qsort_with_data (property_specs, n_property_specs, sizeof (gpointer), + (GCompareDataFunc) _enumerate_values_sort, &type); + } + + for (i = 0; i < n_property_specs; i++) { GParamSpec *prop_spec = property_specs[i]; GValue value = G_VALUE_INIT; @@ -1639,8 +1648,6 @@ nm_setting_enumerate_values (NMSetting *setting, func (setting, prop_spec->name, &value, prop_spec->flags, user_data); g_value_unset (&value); } - - g_free (property_specs); } /** @@ -1656,16 +1663,18 @@ nm_setting_enumerate_values (NMSetting *setting, gboolean _nm_setting_clear_secrets (NMSetting *setting) { - gs_free GParamSpec **property_specs = NULL; - guint n_property_specs; - guint i; + const NMSettInfoSetting *sett_info; gboolean changed = FALSE; + guint i; g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); - property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (setting), &n_property_specs); - for (i = 0; i < n_property_specs; i++) { - GParamSpec *prop_spec = property_specs[i]; + sett_info = _nm_sett_info_setting_get (NM_SETTING_GET_CLASS (setting)); + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + + if (!prop_spec) + continue; if (prop_spec->flags & NM_SETTING_PARAM_SECRET) { GValue value = G_VALUE_INIT; @@ -1730,23 +1739,27 @@ _nm_setting_clear_secrets_with_flags (NMSetting *setting, NMSettingClearSecretsWithFlagsFn func, gpointer user_data) { - gs_free GParamSpec **property_specs = NULL; - guint n_property_specs; - guint i; + const NMSettInfoSetting *sett_info; gboolean changed = FALSE; + guint i; - g_return_val_if_fail (setting, FALSE); g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); g_return_val_if_fail (func != NULL, FALSE); - property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (setting), &n_property_specs); - for (i = 0; i < n_property_specs; i++) { - if (property_specs[i]->flags & NM_SETTING_PARAM_SECRET) { - changed |= NM_SETTING_GET_CLASS (setting)->clear_secrets_with_flags (setting, - property_specs[i], - func, - user_data); - } + sett_info = _nm_sett_info_setting_get (NM_SETTING_GET_CLASS (setting)); + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + + if (!prop_spec) + continue; + + if (!NM_FLAGS_HAS (prop_spec->flags, NM_SETTING_PARAM_SECRET)) + continue; + + changed |= NM_SETTING_GET_CLASS (setting)->clear_secrets_with_flags (setting, + prop_spec, + func, + user_data); } return changed; } From 9c139b2c478c25ea8d313acb279c7bfbbe858303 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 12:02:20 +0100 Subject: [PATCH 13/19] libnm: cleanup NMSettingVpn's get_secret_flags() - most of the time, the secret-name is short and fits in a stack-allocated buffer. Optimize for that by using nm_construct_name_a(). - use _nm_utils_ascii_str_to_int64() instead of strtoul(). tmp = strtoul ((const char *) val, NULL, 10); if ((errno != 0) || (tmp > NM_SETTING_SECRET_FLAGS_ALL)) { is not the right way to check for errors of strtoul(). - refactor the code to return-early on errors. - since commit 9b96bfaa72 "setting-vpn: whatever is in vpn.secrets always is a secrets", we accept secrets without secret-flags as valid too. However, only do that, when we at least have a corresponding key in priv->secrets hash. If the secret name is not used at all, it's clearly not a secret. - if the secret flags are not a valid number, pretend that the flags are still set to "none" (zero). That is because we use the presence of the "*-flags" data item as indication that this is in fact a secret. The user cannot use data items with such a name for another purpose, so on failure, we still claim that this is in fact a secret. --- libnm-core/nm-setting-vpn.c | 47 +++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index fcbeec6854..87c30f7b40 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -647,29 +647,42 @@ get_secret_flags (NMSetting *setting, GError **error) { NMSettingVpnPrivate *priv = NM_SETTING_VPN_GET_PRIVATE (setting); - gs_free char *flags_key = NULL; - gpointer val; - unsigned long tmp; - NMSettingSecretFlags flags = NM_SETTING_SECRET_FLAG_NONE; + gs_free char *flags_key_free = NULL; + const char *flags_key; + const char *flags_val; + gint64 i64; - flags_key = g_strdup_printf ("%s-flags", secret_name); - if (g_hash_table_lookup_extended (priv->data, flags_key, NULL, &val)) { - errno = 0; - tmp = strtoul ((const char *) val, NULL, 10); - if ((errno != 0) || (tmp > NM_SETTING_SECRET_FLAGS_ALL)) { - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("failed to convert value '%s' to uint"), - (const char *) val); + flags_key = nm_construct_name_a ("%s-flags", secret_name, &flags_key_free); + + if (!g_hash_table_lookup_extended (priv->data, flags_key, NULL, (gpointer *) &flags_val)) { + NM_SET_OUT (out_flags, NM_SETTING_SECRET_FLAG_NONE); + + /* having no secret flag for the secret is fine, as long as there + * is the secret itself... */ + if ( verify_secret + && !g_hash_table_contains (priv->secrets, secret_name)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_PROPERTY_NOT_SECRET, + _("secret flags property not found")); g_prefix_error (error, "%s.%s: ", NM_SETTING_VPN_SETTING_NAME, flags_key); return FALSE; } - flags = (NMSettingSecretFlags) tmp; + return TRUE; } - if (out_flags) - *out_flags = flags; + i64 = _nm_utils_ascii_str_to_int64 (flags_val, 10, 0, NM_SETTING_SECRET_FLAGS_ALL, -1); + if (i64 == -1) { + /* The flags keys is set to an unexpected value. That is a configuration + * error. Note that keys named "*-flags" are reserved for secrets. The user + * must not use this for anything but secret flags. Hence, we cannot fail + * to read the secret, we pretend that the secret flag is set to the default + * NM_SETTING_SECRET_FLAG_NONE. */ + NM_SET_OUT (out_flags, NM_SETTING_SECRET_FLAG_NONE); + return TRUE; + } + + NM_SET_OUT (out_flags, (NMSettingSecretFlags) i64); return TRUE; } From 4a5514dc0fb6d3f9b07a14e0e7cda8800f476125 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 6 Jan 2019 13:49:46 +0100 Subject: [PATCH 14/19] libnm: cleanup secret-flags setter and getter There are 3 kinds of secret flag implementations: 1) regular properties have a GObject property and a corresponding "-flags" property. 2) NMSettingVpn handles this entirely differently 3) NMSettingWirelessSecurity's WEP keys, where the secret keys share a flags property that does not follow the same naming scheme as 1). The getter and setter had a boolean "verifiy_secret", only to handle 3). Drop that parameter. Don't let NMSettingWirelessSecurity call the parent's implementation for WEP keys. Just let it handle it directly. --- libnm-core/nm-keyfile.c | 3 +- libnm-core/nm-setting-private.h | 5 ++ libnm-core/nm-setting-vpn.c | 5 +- libnm-core/nm-setting-wireless-security.c | 63 +++++++------- libnm-core/nm-setting.c | 101 ++++++++++++++-------- libnm-core/nm-setting.h | 2 - 6 files changed, 106 insertions(+), 73 deletions(-) diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index 7617da44e9..a781b238f1 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -1948,7 +1948,8 @@ write_hash_of_string (GKeyFile *file, if (vpn_secrets) { NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; - nm_setting_get_secret_flags (setting, property, &secret_flags, NULL); + if (!nm_setting_get_secret_flags (setting, property, &secret_flags, NULL)) + nm_assert_not_reached (); if (!_secret_flags_persist_secret (secret_flags)) continue; } diff --git a/libnm-core/nm-setting-private.h b/libnm-core/nm-setting-private.h index c785fbc90f..c5a11b7a02 100644 --- a/libnm-core/nm-setting-private.h +++ b/libnm-core/nm-setting-private.h @@ -109,6 +109,11 @@ NMSetting *_nm_setting_new_from_dbus (GType setting_type, NMSettingParseFlags parse_flags, GError **error); +gboolean _nm_setting_property_is_regular_secret (NMSetting *setting, + const char *secret_name); +gboolean _nm_setting_property_is_regular_secret_flags (NMSetting *setting, + const char *secret_flags_name); + /*****************************************************************************/ static inline GArray * diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index 87c30f7b40..754c9555b8 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -642,7 +642,6 @@ update_one_secret (NMSetting *setting, const char *key, GVariant *value, GError static gboolean get_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags *out_flags, GError **error) { @@ -659,8 +658,7 @@ get_secret_flags (NMSetting *setting, /* having no secret flag for the secret is fine, as long as there * is the secret itself... */ - if ( verify_secret - && !g_hash_table_contains (priv->secrets, secret_name)) { + if (!g_hash_table_contains (priv->secrets, secret_name)) { g_set_error_literal (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_PROPERTY_NOT_SECRET, @@ -689,7 +687,6 @@ get_secret_flags (NMSetting *setting, static gboolean set_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags flags, GError **error) { diff --git a/libnm-core/nm-setting-wireless-security.c b/libnm-core/nm-setting-wireless-security.c index 38f4519b77..36a00b7c50 100644 --- a/libnm-core/nm-setting-wireless-security.c +++ b/libnm-core/nm-setting-wireless-security.c @@ -1177,53 +1177,56 @@ verify_secrets (NMSetting *setting, NMConnection *connection, GError **error) static gboolean get_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags *out_flags, GError **error) { - NMSettingClass *setting_class; - gboolean verify_override = verify_secret; + NMSettingSecretFlags flags; - /* There's only one 'flags' property for WEP keys, so alias all the WEP key - * property names to that flags property. - */ - if ( !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY0) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY1) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY2) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY3)) { - secret_name = "wep-key"; - verify_override = FALSE; /* Already know it's a secret */ + if (NM_IN_STRSET (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY0, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY1, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY2, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY3)) { + /* There's only one 'flags' property for WEP keys, so alias all the WEP key + * property names to that flags property. */ + nm_assert (_nm_setting_property_is_regular_secret (setting, secret_name)); + nm_assert (_nm_setting_property_is_regular_secret_flags (setting, NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS)); + + g_object_get (G_OBJECT (setting), + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, + &flags, + NULL); + NM_SET_OUT (out_flags, flags); + return TRUE; } - /* Chain up to superclass with modified key name */ - setting_class = NM_SETTING_CLASS (nm_setting_wireless_security_parent_class); - return setting_class->get_secret_flags (setting, secret_name, verify_override, out_flags, error); + return NM_SETTING_CLASS (nm_setting_wireless_security_parent_class)->get_secret_flags (setting, secret_name, out_flags, error); } static gboolean set_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags flags, GError **error) { - NMSettingClass *setting_class; - gboolean verify_override = verify_secret; + if (NM_IN_STRSET (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY0, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY1, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY2, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY3)) { + /* There's only one 'flags' property for WEP keys, so alias all the WEP key + * property names to that flags property. */ + nm_assert (_nm_setting_property_is_regular_secret (setting, secret_name)); + nm_assert (_nm_setting_property_is_regular_secret_flags (setting, NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS)); - /* There's only one 'flags' property for WEP keys, so alias all the WEP key - * property names to that flags property. - */ - if ( !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY0) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY1) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY2) - || !g_strcmp0 (secret_name, NM_SETTING_WIRELESS_SECURITY_WEP_KEY3)) { - secret_name = "wep-key"; - verify_override = FALSE; /* Already know it's a secret */ + if (!nm_g_object_set_property_flags (G_OBJECT (setting), + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, + NM_TYPE_SETTING_SECRET_FLAGS, + flags, + error)) + g_return_val_if_reached (FALSE); + return TRUE; } - /* Chain up to superclass with modified key name */ - setting_class = NM_SETTING_CLASS (nm_setting_wireless_security_parent_class); - return setting_class->set_secret_flags (setting, secret_name, verify_override, flags, error); + return NM_SETTING_CLASS (nm_setting_wireless_security_parent_class)->set_secret_flags (setting, secret_name, flags, error); } static void diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index d8b8095865..1a3e80b499 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1884,52 +1884,72 @@ _nm_setting_update_secrets (NMSetting *setting, GVariant *secrets, GError **erro return result; } -static gboolean -is_secret_prop (NMSetting *setting, const char *secret_name, GError **error) +static void +_set_error_secret_property_not_found (GError **error, + NMSetting *setting, + const char *secret_name) +{ + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_PROPERTY_NOT_FOUND, + _("not a secret property")); + g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), secret_name); +} + +gboolean +_nm_setting_property_is_regular_secret (NMSetting *setting, + const char *secret_name) { const NMSettInfoProperty *property; - GParamSpec *pspec; + + nm_assert (NM_IS_SETTING (setting)); + nm_assert (secret_name); property = _nm_sett_info_property_get (NM_SETTING_GET_CLASS (setting), secret_name); - if (!property) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_PROPERTY_NOT_FOUND, - _("secret is not set")); - g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), secret_name); - return FALSE; - } + return property + && property->param_spec + && NM_FLAGS_HAS (property->param_spec->flags, NM_SETTING_PARAM_SECRET); +} - pspec = property->param_spec; - if (!pspec || !(pspec->flags & NM_SETTING_PARAM_SECRET)) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_PROPERTY_NOT_SECRET, - _("not a secret property")); - g_prefix_error (error, "%s.%s: ", nm_setting_get_name (setting), secret_name); - return FALSE; - } +gboolean +_nm_setting_property_is_regular_secret_flags (NMSetting *setting, + const char *secret_flags_name) +{ + const NMSettInfoProperty *property; - return TRUE; + nm_assert (NM_IS_SETTING (setting)); + nm_assert (secret_flags_name); + + property = _nm_sett_info_property_get (NM_SETTING_GET_CLASS (setting), secret_flags_name); + return property + && property->param_spec + && !NM_FLAGS_HAS (property->param_spec->flags, NM_SETTING_PARAM_SECRET) + && G_PARAM_SPEC_VALUE_TYPE (property->param_spec) == NM_TYPE_SETTING_SECRET_FLAGS; } static gboolean get_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags *out_flags, GError **error) { - gs_free char *name_to_free = NULL; - NMSettingSecretFlags flags = NM_SETTING_SECRET_FLAG_NONE; + gs_free char *secret_flags_name_free = NULL; + const char *secret_flags_name; + NMSettingSecretFlags flags; - if (verify_secret && !is_secret_prop (setting, secret_name, error)) { + if (!_nm_setting_property_is_regular_secret (setting, + secret_name)) { + _set_error_secret_property_not_found (error, setting, secret_name); NM_SET_OUT (out_flags, NM_SETTING_SECRET_FLAG_NONE); return FALSE; } + secret_flags_name = nm_construct_name_a ("%s-flags", secret_name, &secret_flags_name_free); + + nm_assert (_nm_setting_property_is_regular_secret_flags (setting, secret_flags_name)); + g_object_get (G_OBJECT (setting), - nm_construct_name_a ("%s-flags", secret_name, &name_to_free), + secret_flags_name, &flags, NULL); NM_SET_OUT (out_flags, flags); @@ -1958,25 +1978,34 @@ nm_setting_get_secret_flags (NMSetting *setting, g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); g_return_val_if_fail (secret_name != NULL, FALSE); - return NM_SETTING_GET_CLASS (setting)->get_secret_flags (setting, secret_name, TRUE, out_flags, error); + return NM_SETTING_GET_CLASS (setting)->get_secret_flags (setting, secret_name, out_flags, error); } static gboolean set_secret_flags (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags flags, GError **error) { - gs_free char *name_to_free = NULL; + gs_free char *secret_flags_name_free = NULL; + const char *secret_flags_name; - if (verify_secret) - g_return_val_if_fail (is_secret_prop (setting, secret_name, error), FALSE); + if (!_nm_setting_property_is_regular_secret (setting, + secret_name)) { + _set_error_secret_property_not_found (error, setting, secret_name); + return FALSE; + } - g_object_set (G_OBJECT (setting), - nm_construct_name_a ("%s-flags", secret_name, &name_to_free), - flags, - NULL); + secret_flags_name = nm_construct_name_a ("%s-flags", secret_name, &secret_flags_name_free); + + nm_assert (_nm_setting_property_is_regular_secret_flags (setting, secret_flags_name)); + + if (!nm_g_object_set_property_flags (G_OBJECT (setting), + secret_flags_name, + NM_TYPE_SETTING_SECRET_FLAGS, + flags, + error)) + g_return_val_if_reached (FALSE); return TRUE; } @@ -2003,7 +2032,7 @@ nm_setting_set_secret_flags (NMSetting *setting, g_return_val_if_fail (secret_name != NULL, FALSE); g_return_val_if_fail (flags <= NM_SETTING_SECRET_FLAGS_ALL, FALSE); - return NM_SETTING_GET_CLASS (setting)->set_secret_flags (setting, secret_name, TRUE, flags, error); + return NM_SETTING_GET_CLASS (setting)->set_secret_flags (setting, secret_name, flags, error); } /** diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index d6876f86d2..e3c128ffb2 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -191,13 +191,11 @@ typedef struct { gboolean (*get_secret_flags) (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags *out_flags, GError **error); gboolean (*set_secret_flags) (NMSetting *setting, const char *secret_name, - gboolean verify_secret, NMSettingSecretFlags flags, GError **error); From 7771473f46d30127075ebd5166d504a6197d8cdd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 11:28:27 +0100 Subject: [PATCH 15/19] libnm,core: add _nm_connection_aggregate() to replace nm_connection_for_each_setting_value() We should no longer use nm_connection_for_each_setting_value() and nm_setting_for_each_value(). It's fundamentally broken as it does not work with properties that are not backed by a GObject property and it cannot be fixed because it is public API. Add an internal function _nm_connection_aggregate() to replace it. Compare the implementation of the aggregation functionality inside libnm with the previous two checks for secret-flags that it replaces: - previous approach broke abstraction and require detailed knowledge of secret flags. Meaning, they must special case NMSettingVpn and GObject-property based secrets. If we implement a new way for implementing secrets (like we will need for WireGuard), then this the new way should only affect libnm-core, not require changes elsewhere. - it's very inefficient to itereate over all settings. It involves cloning and sorting the list of settings, and retrieve and clone all GObject properties. Only to look at secret properties alone. _nm_connection_aggregate() is supposed to be more flexible then just the two new aggregate types that perform a "find-any" search. The @arg argument and boolean return value can suffice to implement different aggregation types in the future. Also fixes the check of NMAgentManager for secret flags for VPNs (NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs is a property that either has a secret or a secret-flag. The previous implementation would only look at present secrets and check their flags. It wouldn't check secret-flags that are NM_SETTING_SECRET_FLAG_NONE, but have no secret. --- libnm-core/nm-connection.c | 67 ++++++++++++++++++++++++++ libnm-core/nm-core-internal.h | 20 ++++++++ libnm-core/nm-setting-private.h | 7 +++ libnm-core/nm-setting-vpn.c | 60 +++++++++++++++++++++++ libnm-core/nm-setting.c | 69 +++++++++++++++++++++++++++ libnm-core/tests/test-general.c | 51 ++++++++++++++++++-- src/settings/nm-agent-manager.c | 46 +----------------- src/settings/nm-settings-connection.c | 34 +------------ 8 files changed, 274 insertions(+), 80 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index eb42bc07a4..9e5de7b267 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -2033,6 +2033,73 @@ nm_connection_for_each_setting_value (NMConnection *connection, nm_setting_enumerate_values (settings[i], func, user_data); } +/** + * _nm_connection_aggregate: + * @connecition: the #NMConnection for which values are to be aggregated. + * @type: one of the supported aggrate types. + * @arg: the input/output argument that depends on @type. + * + * For example, with %NM_CONNECTION_AGGREGATE_ANY_SECRETS and + * %NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS @arg is a boolean + * output argument. It is either %NULL or a pointer to an gboolean + * out-argument. The function will always set @arg if given. + * Also, the return value of the function is likewise the result + * that is set to @arg. + * + * Returns: a boolean result with the meaning depending on the aggregation + * type @type. + */ +gboolean +_nm_connection_aggregate (NMConnection *connection, + NMConnectionAggregateType type, + gpointer arg) +{ + NMConnectionPrivate *priv; + GHashTableIter iter; + NMSetting *setting; + gboolean arg_boolean; + gboolean completed_early; + gpointer my_arg; + + g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); + + switch (type) { + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + arg_boolean = FALSE; + my_arg = &arg_boolean; + goto good; + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + arg_boolean = FALSE; + my_arg = &arg_boolean; + goto good; + } + g_return_val_if_reached (FALSE); + +good: + priv = NM_CONNECTION_GET_PRIVATE (connection); + + completed_early = FALSE; + g_hash_table_iter_init (&iter, priv->settings); + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &setting)) { + if (_nm_setting_aggregate (setting, type, my_arg)) { + completed_early = TRUE; + break; + } + nm_assert ( my_arg != &arg_boolean + || !arg_boolean); + } + + if (my_arg == &arg_boolean) { + nm_assert (completed_early == arg_boolean); + if (arg) + *((gboolean *) arg) = arg_boolean; + return arg_boolean; + } + + nm_assert_not_reached (); + return FALSE; +} + /** * nm_connection_dump: * @connection: the #NMConnection diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index ec9b108318..d661e49010 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -146,6 +146,26 @@ gpointer _nm_connection_check_main_setting (NMConnection *connection, const char *setting_name, GError **error); +typedef enum { + /* whether the connection has any secrets. + * + * @arg may be %NULL or a pointer to a gboolean for the result. The return + * value of _nm_connection_aggregate() is likewise the boolean result. */ + NM_CONNECTION_AGGREGATE_ANY_SECRETS, + + /* whether the connection has any secret with flags NM_SETTING_SECRET_FLAG_NONE. + * Note that this only cares about the flags, not whether the secret is actually + * present. + * + * @arg may be %NULL or a pointer to a gboolean for the result. The return + * value of _nm_connection_aggregate() is likewise the boolean result. */ + NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, +} NMConnectionAggregateType; + +gboolean _nm_connection_aggregate (NMConnection *connection, + NMConnectionAggregateType type, + gpointer arg); + /** * NMSettingVerifyResult: * @NM_SETTING_VERIFY_SUCCESS: the setting verifies successfully diff --git a/libnm-core/nm-setting-private.h b/libnm-core/nm-setting-private.h index c5a11b7a02..49d27336fa 100644 --- a/libnm-core/nm-setting-private.h +++ b/libnm-core/nm-setting-private.h @@ -97,6 +97,13 @@ gboolean _nm_setting_verify_secret_string (const char *str, const char *property, GError **error); +gboolean _nm_setting_aggregate (NMSetting *setting, + NMConnectionAggregateType type, + gpointer arg); +gboolean _nm_setting_vpn_aggregate (NMSettingVpn *setting, + NMConnectionAggregateType type, + gpointer arg); + gboolean _nm_setting_slave_type_is_valid (const char *slave_type, const char **out_port_type); GVariant *_nm_setting_to_dbus (NMSetting *setting, diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index 754c9555b8..5d488e7f07 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -461,6 +461,66 @@ nm_setting_vpn_foreach_secret (NMSettingVpn *setting, foreach_item_helper (setting, TRUE, func, user_data); } +gboolean +_nm_setting_vpn_aggregate (NMSettingVpn *setting, + NMConnectionAggregateType type, + gpointer arg) +{ + NMSettingVpnPrivate *priv; + NMSettingSecretFlags secret_flags; + const char *key_name; + GHashTableIter iter; + + g_return_val_if_fail (NM_IS_SETTING_VPN (setting), FALSE); + + priv = NM_SETTING_VPN_GET_PRIVATE (setting); + + switch (type) { + + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + if (g_hash_table_size (priv->secrets) > 0) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + return FALSE; + + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + + g_hash_table_iter_init (&iter, priv->secrets); + while (g_hash_table_iter_next (&iter, (gpointer *) &key_name, NULL)) { + if (!nm_setting_get_secret_flags (NM_SETTING (setting), key_name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + } + + /* Ok, we have no secrets with system-secret flags. + * But do we have any secret-flags (without secrets) that indicate system secrets? */ + g_hash_table_iter_init (&iter, priv->data); + while (g_hash_table_iter_next (&iter, (gpointer *) &key_name, NULL)) { + gs_free char *secret_name = NULL; + + if (!g_str_has_suffix (key_name, "-flags")) + continue; + secret_name = g_strndup (key_name, strlen (key_name) - NM_STRLEN ("-flags")); + if (secret_name[0] == '\0') + continue; + if (!nm_setting_get_secret_flags (NM_SETTING (setting), secret_name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + } + + return FALSE; + } + + g_return_val_if_reached (FALSE); +} + /** * nm_setting_vpn_get_timeout: * @setting: the #NMSettingVpn diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 1a3e80b499..8c4fea5045 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1650,6 +1650,75 @@ nm_setting_enumerate_values (NMSetting *setting, } } +/** + * _nm_setting_aggregate: + * @setting: the #NMSetting to aggregate. + * @type: the #NMConnectionAggregateType aggregate type. + * @arg: the in/out arguments for aggregation. They depend on @type. + * + * This is the implementation detail of _nm_connection_aggregate(). It + * makes no sense to call this function directly outside of _nm_connection_aggregate(). + * + * Returns: %TRUE if afterwards the aggregation is complete. That means, + * the only caller _nm_connection_aggregate() will not visit other settings + * after a setting returns %TRUE (indicating that there is nothing further + * to aggregate). Note that is very different from the boolean return + * argument of _nm_connection_aggregate(), which serves a different purpose. + */ +gboolean +_nm_setting_aggregate (NMSetting *setting, + NMConnectionAggregateType type, + gpointer arg) +{ + const NMSettInfoSetting *sett_info; + guint i; + + g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); + g_return_val_if_fail (arg, FALSE); + g_return_val_if_fail (NM_IN_SET (type, NM_CONNECTION_AGGREGATE_ANY_SECRETS, + NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS), + FALSE); + + if (NM_IS_SETTING_VPN (setting)) + return _nm_setting_vpn_aggregate (NM_SETTING_VPN (setting), type, arg); + + sett_info = _nm_sett_info_setting_get (NM_SETTING_GET_CLASS (setting)); + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + nm_auto_unset_gvalue GValue value = G_VALUE_INIT; + NMSettingSecretFlags secret_flags; + + if (!prop_spec) + continue; + if (!NM_FLAGS_HAS (prop_spec->flags, NM_SETTING_PARAM_SECRET)) + continue; + + switch (type) { + + case NM_CONNECTION_AGGREGATE_ANY_SECRETS: + g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (prop_spec)); + g_object_get_property (G_OBJECT (setting), prop_spec->name, &value); + if (!g_param_value_defaults (prop_spec, &value)) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + break; + + case NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS: + if (!nm_setting_get_secret_flags (setting, prop_spec->name, &secret_flags, NULL)) + nm_assert_not_reached (); + if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) { + *((gboolean *) arg) = TRUE; + return TRUE; + } + break; + + } + } + + return FALSE; +} + /** * _nm_setting_clear_secrets: * @setting: the #NMSetting diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 91f116de43..b6dd532616 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -755,10 +755,12 @@ vpn_check_empty_func (const char *key, const char *value, gpointer user_data) static void test_setting_vpn_items (void) { - gs_unref_object NMSettingVpn *s_vpn = NULL; + gs_unref_object NMConnection *connection = NULL; + NMSettingVpn *s_vpn; - s_vpn = (NMSettingVpn *) nm_setting_vpn_new (); - g_assert (s_vpn); + connection = nmtst_create_minimal_connection ("vpn-items", NULL, NM_SETTING_VPN_SETTING_NAME, NULL); + + s_vpn = nm_connection_get_setting_vpn (connection); nm_setting_vpn_add_data_item (s_vpn, "foobar1", "blahblah1"); nm_setting_vpn_add_data_item (s_vpn, "foobar2", "blahblah2"); @@ -772,7 +774,14 @@ test_setting_vpn_items (void) nm_setting_vpn_remove_data_item (s_vpn, "foobar3"); nm_setting_vpn_remove_data_item (s_vpn, "foobar4"); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_add_secret (s_vpn, "foobar1", "blahblah1"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_add_secret (s_vpn, "foobar2", "blahblah2"); nm_setting_vpn_add_secret (s_vpn, "foobar3", "blahblah3"); nm_setting_vpn_add_secret (s_vpn, "foobar4", "blahblah4"); @@ -782,8 +791,25 @@ test_setting_vpn_items (void) nm_setting_vpn_remove_secret (s_vpn, "foobar1"); nm_setting_vpn_remove_secret (s_vpn, "foobar2"); nm_setting_vpn_remove_secret (s_vpn, "foobar3"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_add_data_item (s_vpn, "foobar4-flags", "blahblah4"); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_add_data_item (s_vpn, "foobar4-flags", "2"); + + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + nm_setting_vpn_remove_secret (s_vpn, "foobar4"); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + nm_setting_vpn_remove_data_item (s_vpn, "foobar4-flags"); + /* Try to add some blank values and make sure they are rejected */ NMTST_EXPECT_LIBNM_CRITICAL (NMTST_G_RETURN_MSG (key != NULL)); nm_setting_vpn_add_data_item (s_vpn, NULL, NULL); @@ -1632,6 +1658,25 @@ test_connection_to_dbus_setting_name (void) s_wsec = make_test_wsec_setting ("connection-to-dbus-setting-name"); nm_connection_add_setting (connection, NM_SETTING (s_wsec)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + g_object_set (s_wsec, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NOT_SAVED, + NULL); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (!_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + + g_object_set (s_wsec, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_FLAGS, NM_SETTING_SECRET_FLAG_NONE, + NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD_FLAGS, NM_SETTING_SECRET_FLAG_NONE, + NULL); + + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)); + g_assert (_nm_connection_aggregate (connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL)); + dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); /* Make sure the keys of the first level dict are setting names, not diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 953d3ce459..edadee14bf 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -1058,49 +1058,6 @@ _con_get_request_start_validated (NMAuthChain *chain, nm_auth_chain_destroy (chain); } -static void -has_system_secrets_check (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) -{ - NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; - gboolean *has_system = user_data; - - if (!(flags & NM_SETTING_PARAM_SECRET)) - return; - - /* Clear out system-owned or always-ask secrets */ - if (NM_IS_SETTING_VPN (setting) && !strcmp (key, NM_SETTING_VPN_SECRETS)) { - GHashTableIter iter; - const char *secret_name = NULL; - - /* VPNs are special; need to handle each secret separately */ - g_hash_table_iter_init (&iter, (GHashTable *) g_value_get_boxed (value)); - while (g_hash_table_iter_next (&iter, (gpointer *) &secret_name, NULL)) { - secret_flags = NM_SETTING_SECRET_FLAG_NONE; - nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL); - if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) - *has_system = TRUE; - } - } else { - if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL)) - g_return_if_reached (); - if (secret_flags == NM_SETTING_SECRET_FLAG_NONE) - *has_system = TRUE; - } -} - -static gboolean -has_system_secrets (NMConnection *connection) -{ - gboolean has_system = FALSE; - - nm_connection_for_each_setting_value (connection, has_system_secrets_check, &has_system); - return has_system; -} - static void _con_get_request_start (Request *req) { @@ -1121,7 +1078,8 @@ _con_get_request_start (Request *req) * unprivileged users. */ if ( (req->con.get.flags != NM_SECRET_AGENT_GET_SECRETS_FLAG_NONE) - && (req->con.get.existing_secrets || has_system_secrets (req->con.connection))) { + && ( req->con.get.existing_secrets + || _nm_connection_aggregate (req->con.connection, NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS, NULL))) { _LOGD (NULL, "("LOG_REQ_FMT") request has system secrets; checking agent %s for MODIFY", LOG_REQ_ARG (req), agent_dbus_owner); diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 392823f399..0beb5ea770 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1659,38 +1659,6 @@ typedef struct { bool is_update2:1; } UpdateInfo; -static void -has_some_secrets_cb (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) -{ - GParamSpec *pspec; - - if (NM_IS_SETTING_VPN (setting)) { - if (nm_setting_vpn_get_num_secrets (NM_SETTING_VPN(setting))) - *((gboolean *) user_data) = TRUE; - return; - } - - pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (G_OBJECT (setting)), key); - if (pspec) { - if ( (flags & NM_SETTING_PARAM_SECRET) - && !g_param_value_defaults (pspec, (GValue *)value)) - *((gboolean *) user_data) = TRUE; - } -} - -static gboolean -any_secrets_present (NMConnection *self) -{ - gboolean has_secrets = FALSE; - - nm_connection_for_each_setting_value (self, has_some_secrets_cb, &has_secrets); - return has_secrets; -} - static void cached_secrets_to_connection (NMSettingsConnection *self, NMConnection *connection) { @@ -1758,7 +1726,7 @@ update_auth_cb (NMSettingsConnection *self, } if (info->new_settings) { - if (!any_secrets_present (info->new_settings)) { + if (!_nm_connection_aggregate (info->new_settings, NM_CONNECTION_AGGREGATE_ANY_SECRETS, NULL)) { /* If the new connection has no secrets, we do not want to remove all * secrets, rather we keep all the existing ones. Do that by merging * them in to the new connection. From 7cc2462915286434f900a30c8b6a7b55a4d5b534 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 15:37:39 +0100 Subject: [PATCH 16/19] libnm: move sorting of settings out of nm_setting_enumerate_values() and cache it The property infos are already sorted by name. As nm_setting_enumerate_values() now uses that information, in most cases there is nothing to sort. The only instance is NMSettingConnection, which has a different sort-order. At least for some purposes, not all: - nm_setting_enumerate_values(), obviously. - nm_setting_enumerate_values() is called by keyfile writer. That means, keyfile writer will persist properties in a sorted way. Cache the property list with alternative sorting also in the setting-meta data, instead of calculating it each time. Beside caching the information, this has the additional benefit that this kind of sorting is now directly available, without calling nm_setting_enumerate_values(). Meaning, we can implement keyfile writer without using nm_setting_enumerate_values(). --- libnm-core/nm-core-internal.h | 28 +++++++ libnm-core/nm-setting.c | 143 +++++++++++++++++++++------------- 2 files changed, 115 insertions(+), 56 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index d661e49010..f7abfb6c43 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -659,11 +659,39 @@ typedef struct { struct _NMSettInfoSetting { NMSettingClass *setting_class; + + /* the properties, sorted by property name. */ const NMSettInfoProperty *property_infos; + + /* the @property_infos list is sorted by property name. For some uses we need + * a different sort order. If @property_infos_sorted is set, this is the order + * instead. It is used for: + * + * - nm_setting_enumerate_values() + * - keyfile writer adding keys to the group. + * + * Note that currently only NMSettingConnection implements here a sort order + * that differs from alphabetical sort of the property names. + */ + const NMSettInfoProperty *const*property_infos_sorted; + guint property_infos_len; NMSettInfoSettDetail detail; }; +static inline const NMSettInfoProperty * +_nm_sett_info_property_info_get_sorted (const NMSettInfoSetting *sett_info, + guint idx) +{ + nm_assert (sett_info); + nm_assert (idx < sett_info->property_infos_len); + nm_assert (!sett_info->property_infos_sorted || sett_info->property_infos_sorted[idx]); + + return sett_info->property_infos_sorted + ? sett_info->property_infos_sorted[idx] + : &sett_info->property_infos[idx]; +} + const NMSettInfoSetting *_nm_sett_info_setting_get (NMSettingClass *setting_class); const NMSettInfoProperty *_nm_sett_info_property_get (NMSettingClass *setting_class, diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 8c4fea5045..f79f8c32dc 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -354,6 +354,77 @@ _properties_override_add_transform (GArray *properties_override, static NMSettInfoSetting _sett_info_settings[_NM_META_SETTING_TYPE_NUM]; +static int +_property_infos_sort_cmp_setting_connection (gconstpointer p_a, + gconstpointer p_b, + gpointer user_data) +{ + const NMSettInfoProperty *a = *((const NMSettInfoProperty *const*) p_a); + const NMSettInfoProperty *b = *((const NMSettInfoProperty *const*) p_b); + int c_name; + + c_name = strcmp (a->name, b->name); + nm_assert (c_name != 0); + +#define CMP_AND_RETURN(n_a, n_b, name) \ + G_STMT_START { \ + gboolean _is = nm_streq (n_a, ""name); \ + \ + if ( _is \ + || nm_streq (n_b, ""name)) \ + return _is ? -1 : 1; \ + } G_STMT_END + + /* for [connection], report first id, uuid, type in that order. */ + if (c_name != 0) { + CMP_AND_RETURN (a->name, b->name, NM_SETTING_CONNECTION_ID); + CMP_AND_RETURN (a->name, b->name, NM_SETTING_CONNECTION_UUID); + CMP_AND_RETURN (a->name, b->name, NM_SETTING_CONNECTION_TYPE); + } + +#undef CMP_AND_RETURN + + return c_name; +} + +static const NMSettInfoProperty *const* +_property_infos_sort (const NMSettInfoProperty *property_infos, + guint property_infos_len, + NMSettingClass *setting_class) +{ + const NMSettInfoProperty **arr; + guint i; + +#if NM_MORE_ASSERTS > 5 + /* assert that the property names are all unique and sorted. */ + for (i = 0; i < property_infos_len; i++) { + if (property_infos[i].param_spec) + nm_assert (nm_streq (property_infos[i].name, property_infos[i].param_spec->name)); + if (i > 0) + nm_assert (strcmp (property_infos[i - 1].name, property_infos[i].name) < 0); + } +#endif + + if (property_infos_len <= 1) + return NULL; + if (G_TYPE_FROM_CLASS (setting_class) != NM_TYPE_SETTING_CONNECTION) { + /* we only do something special for certain setting types. This one, + * has just alphabetical sorting. */ + return NULL; + } + + arr = g_new (const NMSettInfoProperty *, property_infos_len); + for (i = 0; i < property_infos_len; i++) + arr[i] = &property_infos[i]; + + g_qsort_with_data (arr, + property_infos_len, + sizeof (const NMSettInfoProperty *), + _property_infos_sort_cmp_setting_connection, + NULL); + return arr; +} + void _nm_setting_class_commit_full (NMSettingClass *setting_class, NMMetaSettingType meta_type, @@ -387,19 +458,21 @@ _nm_setting_class_commit_full (NMSettingClass *setting_class, #if NM_MORE_ASSERTS > 10 /* assert that properties_override is constructed consistently. */ for (i = 0; i < override_len; i++) { - guint j; const NMSettInfoProperty *p = &g_array_index (properties_override, NMSettInfoProperty, i); + gboolean found = FALSE; + guint j; nm_assert (!_nm_sett_info_property_find_in_array ((NMSettInfoProperty *) properties_override->data, i, p->name)); for (j = 0; j < n_property_specs; j++) { - if (nm_streq (property_specs[j]->name, p->name)) { - nm_assert (p->param_spec == property_specs[j]); - break; - } + if (!nm_streq (property_specs[j]->name, p->name)) + continue; + nm_assert (!found); + found = TRUE; + nm_assert (p->param_spec == property_specs[j]); } - nm_assert ((j == n_property_specs) == (p->param_spec == NULL)); + nm_assert (found == (p->param_spec != NULL)); } #endif @@ -429,6 +502,10 @@ _nm_setting_class_commit_full (NMSettingClass *setting_class, sett_info->property_infos_len = properties_override->len; sett_info->property_infos = (const NMSettInfoProperty *) g_array_free (properties_override, properties_override->len == 0); + + sett_info->property_infos_sorted = _property_infos_sort (sett_info->property_infos, + sett_info->property_infos_len, + setting_class); } const NMSettInfoSetting * @@ -1536,33 +1613,6 @@ nm_setting_diff (NMSetting *a, } } -#define CMP_AND_RETURN(n_a, n_b, name) \ - G_STMT_START { \ - gboolean _is = (strcmp (n_a, ""name) == 0); \ - \ - if (_is || (strcmp (n_b, ""name) == 0)) \ - return _is ? -1 : 1; \ - } G_STMT_END - -static int -_enumerate_values_sort (GParamSpec **p_a, GParamSpec **p_b, GType *p_type) -{ - const char *n_a = (*p_a)->name; - const char *n_b = (*p_b)->name; - int c = strcmp (n_a, n_b); - - if (c) { - if (*p_type == NM_TYPE_SETTING_CONNECTION) { - /* for [connection], report first id, uuid, type in that order. */ - CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_ID); - CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_UUID); - CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_TYPE); - } - } - return c; -} -#undef CMP_AND_RETURN - /** * nm_setting_enumerate_values: * @setting: the #NMSetting @@ -1578,10 +1628,7 @@ nm_setting_enumerate_values (NMSetting *setting, gpointer user_data) { const NMSettInfoSetting *sett_info; - gs_free GParamSpec **property_specs = NULL; - guint n_property_specs; guint i; - GType type; g_return_if_fail (NM_IS_SETTING (setting)); g_return_if_fail (func != NULL); @@ -1620,29 +1667,13 @@ nm_setting_enumerate_values (NMSetting *setting, return; } - if (sett_info->property_infos_len == 0) - return; - - /* sort the properties. This has an effect on the order in which keyfile - * prints them. */ - property_specs = g_new (GParamSpec *, sett_info->property_infos_len); - n_property_specs = 0; for (i = 0; i < sett_info->property_infos_len; i++) { - GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; - - if (prop_spec) - property_specs[n_property_specs++] = prop_spec; - } - if (n_property_specs > 1) { - type = G_OBJECT_TYPE (setting); - g_qsort_with_data (property_specs, n_property_specs, sizeof (gpointer), - (GCompareDataFunc) _enumerate_values_sort, &type); - } - - for (i = 0; i < n_property_specs; i++) { - GParamSpec *prop_spec = property_specs[i]; + GParamSpec *prop_spec = _nm_sett_info_property_info_get_sorted (sett_info, i)->param_spec; GValue value = G_VALUE_INIT; + if (!prop_spec) + continue; + g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (prop_spec)); g_object_get_property (G_OBJECT (setting), prop_spec->name, &value); func (setting, prop_spec->name, &value, prop_spec->flags, user_data); From 0e09da4ec0d8017393520b8147e69ada085bf532 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 14:37:30 +0100 Subject: [PATCH 17/19] libnm: rework nm_setting_duplicate() without nm_setting_enumerate_values() Drop another use of nm_setting_enumerate_values(). Using nm_setting_enumerate_values() to duplicate a setting already didn't work for gendata based settings. Also, nm_setting_enumerate_values() would iterate the properties in a particular order. We don't need that, the default order (asciibetical sorted) is fine. --- libnm-core/nm-setting.c | 51 ++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index f79f8c32dc..9fbc6be7f5 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1051,14 +1051,19 @@ _nm_setting_get_property (NMSetting *setting, const char *property_name, GValue } static void -duplicate_setting (NMSetting *setting, - const char *name, - const GValue *value, - GParamFlags flags, - gpointer user_data) +_gobject_copy_property (GObject *src, + GObject *dst, + const char *property_name, + GType gtype) { - if ((flags & (G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)) == G_PARAM_WRITABLE) - g_object_set_property (G_OBJECT (user_data), name, value); + nm_auto_unset_gvalue GValue value = G_VALUE_INIT; + + nm_assert (G_IS_OBJECT (src)); + nm_assert (G_IS_OBJECT (dst)); + + g_value_init (&value, gtype); + g_object_get_property (src, property_name, &value); + g_object_set_property (dst, property_name, &value); } /** @@ -1099,11 +1104,35 @@ nm_setting_duplicate (NMSetting *setting) g_variant_ref (val)); } } - } else { - g_object_freeze_notify (dup); - nm_setting_enumerate_values (setting, duplicate_setting, dup); - g_object_thaw_notify (dup); } + + if (sett_info->property_infos_len > 0) { + gboolean frozen = FALSE; + guint i; + + for (i = 0; i < sett_info->property_infos_len; i++) { + GParamSpec *prop_spec = sett_info->property_infos[i].param_spec; + + if (!prop_spec) + continue; + if ((prop_spec->flags & (G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)) != G_PARAM_WRITABLE) + continue; + + if (!frozen) { + g_object_freeze_notify (dup); + frozen = TRUE; + } + + _gobject_copy_property (G_OBJECT (setting), + dup, + prop_spec->name, + G_PARAM_SPEC_VALUE_TYPE (prop_spec)); + } + + if (frozen) + g_object_thaw_notify (dup); + } + return NM_SETTING (dup); } From 038d509695c996b561c94e565eaeae5fc34136b2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Jan 2019 16:14:07 +0100 Subject: [PATCH 18/19] libnm/keyfile: don't use nm_setting_enumerate_values() in keyfile reader/writer - nm_setting_enumerate_values() cannot handle non-GObject based properties as it cannot abstract them. That means, for gendata based settings, we need already a special case, and future ways of handling settings (WireGuard peers) also won't work without special casing. - nm_setting_enumerate_values() needs to fetch all properties, although some properties will not be actually used. E.g. secret properties will be ignored depending on the flag. Or compare the read-side with read_one_setting_value(). The reader doesn't care about the (unset) GObject property. It really just cares about the GType of the proeprty. Still, nm_setting_enumerate_values() fetches all (empty) properties. Or consider, route_writer() as called by nm_setting_enumerate_values() always needs to deep-clone the entire list of routes in the property getter for NM_SETTING_IP_CONFIG_ROUTES, then unpack it. This is unnecesary overhead. This is not yet fixed, but would now be easy to improve. - a for-each function like nm_setting_enumerate_values() does make code harder to read, gives less possibility for optimization, and is in general less flexible. Don't use it. --- libnm-core/nm-keyfile.c | 141 ++++++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 62 deletions(-) diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c index a781b238f1..86a80e1dc7 100644 --- a/libnm-core/nm-keyfile.c +++ b/libnm-core/nm-keyfile.c @@ -2588,27 +2588,27 @@ _parse_info_find (NMSetting *setting, /*****************************************************************************/ static void -read_one_setting_value (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flags, - gpointer user_data) +read_one_setting_value (KeyfileReaderInfo *info, + NMSetting *setting, + const NMSettInfoProperty *property_info) { - KeyfileReaderInfo *info = user_data; GKeyFile *keyfile = info->keyfile; gs_free_error GError *err = NULL; const ParseInfoProperty *pip; gs_free char *tmp_str = NULL; const char *setting_name; + const char *key; GType type; guint64 u64; gint64 i64; - if (info->error) + nm_assert (!info->error); + nm_assert (property_info->param_spec); + + if ((property_info->param_spec->flags & (G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)) != G_PARAM_WRITABLE) return; - if (!(flags & G_PARAM_WRITABLE)) - return; + key = property_info->param_spec->name; pip = _parse_info_find (setting, key, &setting_name); @@ -2643,7 +2643,7 @@ read_one_setting_value (NMSetting *setting, return; } - type = G_VALUE_TYPE (value); + type = G_PARAM_SPEC_VALUE_TYPE (property_info->param_spec); if (type == G_TYPE_STRING) { gs_free char *str_val = NULL; @@ -2754,7 +2754,7 @@ read_one_setting_value (NMSetting *setting, read_hash_of_string (keyfile, setting, key); } else if (type == G_TYPE_ARRAY) { read_array_of_uint (keyfile, setting, key); - } else if (G_VALUE_HOLDS_FLAGS (value)) { + } else if (G_TYPE_IS_FLAGS (type)) { tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); if (!err) { u64 = _nm_utils_ascii_str_to_uint64 (tmp_str, 0, 0, G_MAXUINT, G_MAXUINT64); @@ -2765,7 +2765,7 @@ read_one_setting_value (NMSetting *setting, } else nm_g_object_set_property_flags (G_OBJECT (setting), key, type, u64, &err); } - } else if (G_VALUE_HOLDS_ENUM (value)) { + } else if (G_TYPE_IS_ENUM (type)) { tmp_str = nm_keyfile_plugin_kf_get_value (keyfile, setting_name, key, &err); if (!err) { i64 = _nm_utils_ascii_str_to_int64 (tmp_str, 0, G_MININT, G_MAXINT, G_MAXINT64); @@ -2798,6 +2798,7 @@ _read_setting (KeyfileReaderInfo *info) gs_unref_object NMSetting *setting = NULL; const char *alias; GType type; + guint i; alias = nm_keyfile_plugin_get_setting_name_for_alias (info->group); if (!alias) @@ -2818,7 +2819,7 @@ _read_setting (KeyfileReaderInfo *info) if (sett_info->detail.gendata_info) { gs_free char **keys = NULL; - gsize i, n_keys; + gsize k, n_keys; keys = g_key_file_get_keys (info->keyfile, info->group, &n_keys, NULL); if (!keys) @@ -2827,16 +2828,16 @@ _read_setting (KeyfileReaderInfo *info) GHashTable *h = _nm_setting_gendata_hash (setting, TRUE); nm_utils_strv_sort (keys, n_keys); - for (i = 0; i < n_keys; i++) { - gs_free char *key = keys[i]; + for (k = 0; k < n_keys; k++) { + gs_free char *key = keys[k]; gs_free_error GError *local = NULL; const GVariantType *variant_type; GVariant *variant; /* a GKeyFile can return duplicate keys, there is just no API to make sense * of them. Skip them. */ - if ( i + 1 < n_keys - && nm_streq (key, keys[i + 1])) + if ( k + 1 < n_keys + && nm_streq (key, keys[k + 1])) continue; /* currently, the API is very simple. The setting class just returns @@ -2881,13 +2882,20 @@ _read_setting (KeyfileReaderInfo *info) g_steal_pointer (&key), g_variant_take_ref (variant)); } - for (; i < n_keys; i++) - g_free (keys[i]); + for (; k < n_keys; k++) + g_free (keys[k]); } - goto out; } - nm_setting_enumerate_values (setting, read_one_setting_value, info); + for (i = 0; i < sett_info->property_infos_len; i++) { + const NMSettInfoProperty *property_info = &sett_info->property_infos[i]; + + if (property_info->param_spec) { + read_one_setting_value (info, setting, property_info); + if (info->error) + goto out; + } + } out: info->setting = NULL; @@ -3063,24 +3071,23 @@ out_with_info_error: /*****************************************************************************/ static void -write_setting_value (NMSetting *setting, - const char *key, - const GValue *value, - GParamFlags flag, - gpointer user_data) +write_setting_value (KeyfileWriterInfo *info, + NMSetting *setting, + const NMSettInfoProperty *property_info) { - KeyfileWriterInfo *info = user_data; - const char *setting_name; - GType type; const ParseInfoProperty *pip; - GParamSpec *pspec; + const char *setting_name; + const char *key; char numstr[64]; + GValue value; + GType type; - if (info->error) + nm_assert (!info->error); + + if (!property_info->param_spec) return; - pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (setting), key); - nm_assert (pspec); + key = property_info->param_spec->name; pip = _parse_info_find (setting, key, &setting_name); @@ -3108,7 +3115,7 @@ write_setting_value (NMSetting *setting, * the secret flags there are in a third-level hash in the 'secrets' * property. */ - if ( (pspec->flags & NM_SETTING_PARAM_SECRET) + if ( (property_info->param_spec->flags & NM_SETTING_PARAM_SECRET) && !NM_IS_SETTING_VPN (setting)) { NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE; @@ -3118,50 +3125,55 @@ write_setting_value (NMSetting *setting, return; } + value = (GValue) { 0 }; + + g_value_init (&value, G_PARAM_SPEC_VALUE_TYPE (property_info->param_spec)); + g_object_get_property (G_OBJECT (setting), property_info->param_spec->name, &value); + if ( (!pip || !pip->writer_persist_default) - && g_param_value_defaults (pspec, (GValue *) value)) { + && g_param_value_defaults (property_info->param_spec, &value)) { nm_assert (!g_key_file_has_key (info->keyfile, setting_name, key, NULL)); - return; + goto out_unset_value; } if (pip && pip->writer) { - pip->writer (info, setting, key, value); - return; + pip->writer (info, setting, key, &value); + goto out_unset_value; } - type = G_VALUE_TYPE (value); + type = G_VALUE_TYPE (&value); if (type == G_TYPE_STRING) { const char *str; - str = g_value_get_string (value); + str = g_value_get_string (&value); if (str) nm_keyfile_plugin_kf_set_string (info->keyfile, setting_name, key, str); } else if (type == G_TYPE_UINT) { - nm_sprintf_buf (numstr, "%u", g_value_get_uint (value)); + nm_sprintf_buf (numstr, "%u", g_value_get_uint (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_INT) { - nm_sprintf_buf (numstr, "%d", g_value_get_int (value)); + nm_sprintf_buf (numstr, "%d", g_value_get_int (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_UINT64) { - nm_sprintf_buf (numstr, "%" G_GUINT64_FORMAT, g_value_get_uint64 (value)); + nm_sprintf_buf (numstr, "%" G_GUINT64_FORMAT, g_value_get_uint64 (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_INT64) { - nm_sprintf_buf (numstr, "%" G_GINT64_FORMAT, g_value_get_int64 (value)); + nm_sprintf_buf (numstr, "%" G_GINT64_FORMAT, g_value_get_int64 (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BOOLEAN) { nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, - g_value_get_boolean (value) + g_value_get_boolean (&value) ? "true" : "false"); } else if (type == G_TYPE_CHAR) { - nm_sprintf_buf (numstr, "%d", (int) g_value_get_schar (value)); + nm_sprintf_buf (numstr, "%d", (int) g_value_get_schar (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else if (type == G_TYPE_BYTES) { GBytes *bytes; const guint8 *data; gsize len = 0; - bytes = g_value_get_boxed (value); + bytes = g_value_get_boxed (&value); data = bytes ? g_bytes_get_data (bytes, &len) : NULL; if (data != NULL && len > 0) @@ -3169,20 +3181,23 @@ write_setting_value (NMSetting *setting, } else if (type == G_TYPE_STRV) { char **array; - array = (char **) g_value_get_boxed (value); + array = (char **) g_value_get_boxed (&value); nm_keyfile_plugin_kf_set_string_list (info->keyfile, setting_name, key, (const char **const) array, g_strv_length (array)); } else if (type == G_TYPE_HASH_TABLE) { - write_hash_of_string (info->keyfile, setting, key, value); + write_hash_of_string (info->keyfile, setting, key, &value); } else if (type == G_TYPE_ARRAY) { - write_array_of_uint (info->keyfile, setting, key, value); - } else if (G_VALUE_HOLDS_FLAGS (value)) { - nm_sprintf_buf (numstr, "%u", g_value_get_flags (value)); + write_array_of_uint (info->keyfile, setting, key, &value); + } else if (G_VALUE_HOLDS_FLAGS (&value)) { + nm_sprintf_buf (numstr, "%u", g_value_get_flags (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); - } else if (G_VALUE_HOLDS_ENUM (value)) { - nm_sprintf_buf (numstr, "%d", g_value_get_enum (value)); + } else if (G_VALUE_HOLDS_ENUM (&value)) { + nm_sprintf_buf (numstr, "%d", g_value_get_enum (&value)); nm_keyfile_plugin_kf_set_value (info->keyfile, setting_name, key, numstr); } else g_return_if_reached (); + +out_unset_value: + g_value_unset (&value); } GKeyFile * @@ -3194,7 +3209,7 @@ nm_keyfile_write (NMConnection *connection, gs_unref_keyfile GKeyFile *keyfile = NULL; KeyfileWriterInfo info; gs_free NMSetting **settings = NULL; - guint i, n_settings = 0; + guint i, j, n_settings = 0; g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); g_return_val_if_fail (!error || !*error, NULL); @@ -3252,18 +3267,20 @@ nm_keyfile_write (NMConnection *connection, } } } - goto next; } - nm_setting_enumerate_values (setting, write_setting_value, &info); + for (j = 0; j < sett_info->property_infos_len; j++) { + const NMSettInfoProperty *property_info = _nm_sett_info_property_info_get_sorted (sett_info, j); -next: - if (info.error) - goto out_with_info_error; + write_setting_value (&info, setting, property_info); + if (info.error) + goto out_with_info_error; + } + + nm_assert (!info.error); } - if (info.error) - goto out_with_info_error; + nm_assert (!info.error); return g_steal_pointer (&keyfile); From a5b20ba211f62c4ef29cb34d2a800291ae37e31d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 29 Dec 2018 21:23:09 +0100 Subject: [PATCH 19/19] libnm-core: add _nm_setting_secret_flags_valid() helper Secret-flags are flags, but most combinations don't actually make sense and maybe should be rejected. Anyway, that is not done, and most places just check that there are no unknown flags set. Add _nm_setting_secret_flags_valid() to perform the check at one place instead of having the implementation at various places. --- libnm-core/nm-core-internal.h | 18 ++++++++++++++---- libnm-core/nm-setting-vpn.c | 3 ++- libnm-core/nm-setting.c | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index f7abfb6c43..1e7ec9bcbc 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -121,11 +121,21 @@ */ #define NM_SETTING_COMPARE_FLAG_NONE ((NMSettingCompareFlags) 0) +/*****************************************************************************/ + #define NM_SETTING_SECRET_FLAGS_ALL \ - (NM_SETTING_SECRET_FLAG_NONE | \ - NM_SETTING_SECRET_FLAG_AGENT_OWNED | \ - NM_SETTING_SECRET_FLAG_NOT_SAVED | \ - NM_SETTING_SECRET_FLAG_NOT_REQUIRED) + ((NMSettingSecretFlags) ( NM_SETTING_SECRET_FLAG_NONE \ + | NM_SETTING_SECRET_FLAG_AGENT_OWNED \ + | NM_SETTING_SECRET_FLAG_NOT_SAVED \ + | NM_SETTING_SECRET_FLAG_NOT_REQUIRED)) + +static inline gboolean +_nm_setting_secret_flags_valid (NMSettingSecretFlags flags) +{ + return !NM_FLAGS_ANY (flags, ~NM_SETTING_SECRET_FLAGS_ALL); +} + +/*****************************************************************************/ typedef enum { /*< skip >*/ NM_SETTING_PARSE_FLAGS_NONE = 0, diff --git a/libnm-core/nm-setting-vpn.c b/libnm-core/nm-setting-vpn.c index 5d488e7f07..d485b2a802 100644 --- a/libnm-core/nm-setting-vpn.c +++ b/libnm-core/nm-setting-vpn.c @@ -730,7 +730,8 @@ get_secret_flags (NMSetting *setting, } i64 = _nm_utils_ascii_str_to_int64 (flags_val, 10, 0, NM_SETTING_SECRET_FLAGS_ALL, -1); - if (i64 == -1) { + if ( i64 == -1 + || !_nm_setting_secret_flags_valid (i64)) { /* The flags keys is set to an unexpected value. That is a configuration * error. Note that keys named "*-flags" are reserved for secrets. The user * must not use this for anything but secret flags. Hence, we cannot fail diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 9fbc6be7f5..4662912a89 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2159,7 +2159,7 @@ nm_setting_set_secret_flags (NMSetting *setting, { g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); g_return_val_if_fail (secret_name != NULL, FALSE); - g_return_val_if_fail (flags <= NM_SETTING_SECRET_FLAGS_ALL, FALSE); + g_return_val_if_fail (_nm_setting_secret_flags_valid (flags), FALSE); return NM_SETTING_GET_CLASS (setting)->set_secret_flags (setting, secret_name, flags, error); }