From 7328976a022bec8ad05565a7f32bd410ae566404 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 12:47:11 +0100 Subject: [PATCH 1/7] ifcfg-rh/tests: test writing multiple bond options --- .../network-scripts/ifcfg-Test_Write_Bond_Main.cexpected | 2 +- src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Bond_Main.cexpected b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Bond_Main.cexpected index 854d24905..5d81dfefb 100644 --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Bond_Main.cexpected +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Bond_Main.cexpected @@ -1,4 +1,4 @@ -BONDING_OPTS=mode=balance-rr +BONDING_OPTS="downdelay=5 miimon=100 mode=balance-rr updelay=10" TYPE=Bond BONDING_MASTER=yes PROXY_METHOD=none diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 17b6ca494..10a0f6839 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -8058,6 +8058,10 @@ test_write_bond_main (void) s_bond = (NMSettingBond *) nm_setting_bond_new (); nm_connection_add_setting (connection, NM_SETTING (s_bond)); + nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_DOWNDELAY, "5"); + nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_UPDELAY, "10"); + nm_setting_bond_add_option (s_bond, NM_SETTING_BOND_OPTION_MIIMON, "100"); + /* IP4 setting */ s_ip4 = (NMSettingIPConfig *) nm_setting_ip4_config_new (); nm_connection_add_setting (connection, NM_SETTING (s_ip4)); From 3adce12898aef656ca297c13acca6af2b3259d4b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 12:39:01 +0100 Subject: [PATCH 2/7] shared: add NMUtilsNamedEntry It is common to have some data indexed by a name. If you want to sort a list of such data, you would have to re-implement your own compare function each time. Instead, add NMUtilsNamedEntry which as first field has the name. So, you can create your own struct: struct my_data { const char *name; ... other fields } and compare them with with nm_utils_named_entry_cmp(). For convenience, add another struct NMUtilsNamedValue, which has only one data field, a pointer. --- shared/nm-utils/nm-shared-utils.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index a092a6b78..1e80b35a9 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -407,6 +407,26 @@ char *nm_utils_str_utf8safe_escape_take (char *str, NMUtilsStrUtf8SafeFlags flag /*****************************************************************************/ +typedef struct { + const char *name; +} NMUtilsNamedEntry; + +typedef struct { + union { + NMUtilsNamedEntry named_entry; + const char *name; + }; + union { + const char *value_str; + gconstpointer value_ptr; + }; +} NMUtilsNamedValue; + +#define nm_utils_named_entry_cmp nm_strcmp_p +#define nm_utils_named_entry_cmp_with_data nm_strcmp_p_with_data + +/*****************************************************************************/ + #define NM_UTILS_NS_PER_SECOND ((gint64) 1000000000) #define NM_UTILS_NS_PER_MSEC ((gint64) 1000000) #define NM_UTILS_NS_TO_MSEC_CEIL(nsec) (((nsec) + (NM_UTILS_NS_PER_MSEC - 1)) / NM_UTILS_NS_PER_MSEC) From 02d1ffa9ca83e46d07b929193648a0f82ebb2512 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 13:07:42 +0100 Subject: [PATCH 3/7] libnm/trivial: reorder code in libnm-core/nm-setting-bond.c --- libnm-core/nm-setting-bond.c | 106 ++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 45 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index e85d1564e..cf08c5b05 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -34,6 +34,8 @@ #include "nm-setting-infiniband.h" #include "nm-core-internal.h" +/*****************************************************************************/ + /** * SECTION:nm-setting-bond * @short_description: Describes connection properties for bonds @@ -42,15 +44,7 @@ * necessary for bond connections. **/ -G_DEFINE_TYPE_WITH_CODE (NMSettingBond, nm_setting_bond, NM_TYPE_SETTING, - _nm_register_setting (BOND, NM_SETTING_PRIORITY_HW_BASE)) -NM_SETTING_REGISTER_TYPE (NM_TYPE_SETTING_BOND) - -#define NM_SETTING_BOND_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_SETTING_BOND, NMSettingBondPrivate)) - -typedef struct { - GHashTable *options; -} NMSettingBondPrivate; +/*****************************************************************************/ enum { PROP_0, @@ -58,6 +52,18 @@ enum { LAST_PROP }; +typedef struct { + GHashTable *options; +} NMSettingBondPrivate; + +G_DEFINE_TYPE_WITH_CODE (NMSettingBond, nm_setting_bond, NM_TYPE_SETTING, + _nm_register_setting (BOND, NM_SETTING_PRIORITY_HW_BASE)) +NM_SETTING_REGISTER_TYPE (NM_TYPE_SETTING_BOND) + +#define NM_SETTING_BOND_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_SETTING_BOND, NMSettingBondPrivate)) + +/*****************************************************************************/ + typedef struct { const char *opt; const char *val; @@ -104,18 +110,7 @@ static const BondDefault defaults[] = { { NM_SETTING_BOND_OPTION_LP_INTERVAL, "1", NM_BOND_OPTION_TYPE_INT, 1, G_MAXINT }, }; -/** - * nm_setting_bond_new: - * - * Creates a new #NMSettingBond object with default values. - * - * Returns: (transfer full): the new empty #NMSettingBond object - **/ -NMSetting * -nm_setting_bond_new (void) -{ - return (NMSetting *) g_object_new (NM_TYPE_SETTING_BOND, NULL); -} +/*****************************************************************************/ /** * nm_setting_bond_get_num_options: @@ -514,6 +509,8 @@ _nm_setting_bond_mode_from_string (const char *str) return NM_BOND_MODE_UNKNOWN; } +/*****************************************************************************/ + #define BIT(x) (1 << (x)) static const struct { @@ -812,6 +809,8 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return TRUE; } +/*****************************************************************************/ + static gboolean options_hash_match (NMSettingBond *s_bond, GHashTable *options1, @@ -887,25 +886,22 @@ compare_property (NMSetting *setting, return parent_class->compare_property (setting, other, prop_spec, flags); } -static void -nm_setting_bond_init (NMSettingBond *setting) -{ - NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (setting); - - priv->options = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_free); - - /* Default values: */ - nm_setting_bond_add_option (setting, NM_SETTING_BOND_OPTION_MODE, "balance-rr"); -} +/*****************************************************************************/ static void -finalize (GObject *object) +get_property (GObject *object, guint prop_id, + GValue *value, GParamSpec *pspec) { NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (object); - g_hash_table_destroy (priv->options); - - G_OBJECT_CLASS (nm_setting_bond_parent_class)->finalize (object); + switch (prop_id) { + case PROP_OPTIONS: + g_value_take_boxed (value, _nm_utils_copy_strdict (priv->options)); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } } static void @@ -925,20 +921,40 @@ set_property (GObject *object, guint prop_id, } } +/*****************************************************************************/ + static void -get_property (GObject *object, guint prop_id, - GValue *value, GParamSpec *pspec) +nm_setting_bond_init (NMSettingBond *setting) +{ + NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (setting); + + priv->options = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_free); + + /* Default values: */ + nm_setting_bond_add_option (setting, NM_SETTING_BOND_OPTION_MODE, "balance-rr"); +} + +/** + * nm_setting_bond_new: + * + * Creates a new #NMSettingBond object with default values. + * + * Returns: (transfer full): the new empty #NMSettingBond object + **/ +NMSetting * +nm_setting_bond_new (void) +{ + return (NMSetting *) g_object_new (NM_TYPE_SETTING_BOND, NULL); +} + +static void +finalize (GObject *object) { NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (object); - switch (prop_id) { - case PROP_OPTIONS: - g_value_take_boxed (value, _nm_utils_copy_strdict (priv->options)); - break; - default: - G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); - break; - } + g_hash_table_destroy (priv->options); + + G_OBJECT_CLASS (nm_setting_bond_parent_class)->finalize (object); } static void From d5b3c6ee53cfa965ee0c219331aab9348bf80107 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 13:25:57 +0100 Subject: [PATCH 4/7] libnm: sort entries in nm_setting_bond_get_option() Since the order was arbitrary before, we can also sort it. Also rework it, to avoid the creating a temporary GList of keys. --- libnm-core/nm-setting-bond.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index cf08c5b05..8a15500c2 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -158,26 +158,34 @@ nm_setting_bond_get_option (NMSettingBond *setting, const char **out_value) { NMSettingBondPrivate *priv; - GList *keys; - const char *_key = NULL, *_value = NULL; + guint i, len; + gs_free NMUtilsNamedValue *options = NULL; + GHashTableIter iter; + const char *key, *value; g_return_val_if_fail (NM_IS_SETTING_BOND (setting), FALSE); priv = NM_SETTING_BOND_GET_PRIVATE (setting); - if (idx >= nm_setting_bond_get_num_options (setting)) + len = g_hash_table_size (priv->options); + if (idx >= len) return FALSE; - keys = g_hash_table_get_keys (priv->options); - _key = g_list_nth_data (keys, idx); - _value = g_hash_table_lookup (priv->options, _key); + i = 0; + options = g_new (NMUtilsNamedValue, len); + g_hash_table_iter_init (&iter, priv->options); + while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) { + options[i].name = key; + options[i].value_str = value; + i++; + } + nm_assert (i == len); - if (out_name) - *out_name = _key; - if (out_value) - *out_value = _value; + g_qsort_with_data (options, len, sizeof (options[0]), + nm_utils_named_entry_cmp_with_data, NULL); - g_list_free (keys); + NM_SET_OUT (out_name, options[idx].name); + NM_SET_OUT (out_value, options[idx].value_str); return TRUE; } From 6b319cd0722085ef686b65b24b2c8de72c0fa25b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 12:42:12 +0100 Subject: [PATCH 5/7] ifcfg-rh: avoid duplicate lookup of bond-option in write_bond_setting() Now that nm_setting_bond_get_option() has a stable order (alphabetically), we no longer need to sort it. --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 929264096..c277cb2ad 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1311,21 +1311,14 @@ write_bond_setting (NMConnection *connection, shvarFile *ifcfg, gboolean *wired, num_opts = nm_setting_bond_get_num_options (s_bond); if (num_opts) { nm_auto_free_gstring GString *str = NULL; - gs_free const char **options = NULL; - const char *value; + const char *name, *value; str = g_string_sized_new (64); - options = g_new (const char *, num_opts); - for (i = 0; i < num_opts; i++) - nm_setting_bond_get_option (s_bond, i, &options[i], &value); - g_qsort_with_data (options, num_opts, sizeof (const char *), - nm_strcmp_p_with_data, NULL); - for (i = 0; i < num_opts; i++) { if (str->len) g_string_append_c (str, ' '); - value = nm_setting_bond_get_option_by_name (s_bond, options[i]); - g_string_append_printf (str, "%s=%s", options[i], value); + nm_setting_bond_get_option (s_bond, i, &name, &value); + g_string_append_printf (str, "%s=%s", name, value); } svSetValueStr (ifcfg, "BONDING_OPTS", str->str); From 3c8c63dcca9e6e8e8500d53589a5e8c56b78a4ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 13:35:46 +0100 Subject: [PATCH 6/7] libnm: stable order in _nm_utils_strdict_to_dbus() --- libnm-core/nm-utils.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index c3001ab5d..ef7201139 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -490,17 +490,45 @@ _nm_utils_strdict_to_dbus (const GValue *prop_value) { GHashTable *hash; GHashTableIter iter; - gpointer key, value; + const char *key, *value; GVariantBuilder builder; + guint i, len; g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{ss}")); + hash = g_value_get_boxed (prop_value); - if (hash) { - g_hash_table_iter_init (&iter, hash); - while (g_hash_table_iter_next (&iter, &key, &value)) - g_variant_builder_add (&builder, "{ss}", key, value); + if (!hash) + goto out; + len = g_hash_table_size (hash); + if (!len) + goto out; + + g_hash_table_iter_init (&iter, hash); + if (!g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) + nm_assert_not_reached (); + + if (len == 1) + g_variant_builder_add (&builder, "{ss}", key, value); + else { + gs_free NMUtilsNamedValue *idx = NULL; + + idx = g_new (NMUtilsNamedValue, len); + i = 0; + do { + idx[i].name = key; + idx[i].value_str = value; + i++; + } while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)); + nm_assert (i == len); + + g_qsort_with_data (idx, len, sizeof (idx[0]), + nm_utils_named_entry_cmp_with_data, NULL); + + for (i = 0; i < len; i++) + g_variant_builder_add (&builder, "{ss}", idx[i].name, idx[i].value_str); } +out: return g_variant_builder_end (&builder); } From 7ce8a1e67756e0b23024fc09400a4edcf0fb5be6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 21 Nov 2017 14:01:02 +0100 Subject: [PATCH 7/7] libnm: cache lookup index for nm_setting_bond_get_option() --- libnm-core/nm-setting-bond.c | 42 +++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index 8a15500c2..44219bfe2 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -54,6 +54,7 @@ enum { typedef struct { GHashTable *options; + NMUtilsNamedValue *options_idx_cache; } NMSettingBondPrivate; G_DEFINE_TYPE_WITH_CODE (NMSettingBond, nm_setting_bond, NM_TYPE_SETTING, @@ -159,7 +160,6 @@ nm_setting_bond_get_option (NMSettingBond *setting, { NMSettingBondPrivate *priv; guint i, len; - gs_free NMUtilsNamedValue *options = NULL; GHashTableIter iter; const char *key, *value; @@ -171,21 +171,26 @@ nm_setting_bond_get_option (NMSettingBond *setting, if (idx >= len) return FALSE; - i = 0; - options = g_new (NMUtilsNamedValue, len); - g_hash_table_iter_init (&iter, priv->options); - while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) { - options[i].name = key; - options[i].value_str = value; - i++; + if (!G_UNLIKELY (priv->options_idx_cache)) { + NMUtilsNamedValue *options; + + i = 0; + options = g_new (NMUtilsNamedValue, len); + g_hash_table_iter_init (&iter, priv->options); + while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) { + options[i].name = key; + options[i].value_str = value; + i++; + } + nm_assert (i == len); + + g_qsort_with_data (options, len, sizeof (options[0]), + nm_utils_named_entry_cmp_with_data, NULL); + priv->options_idx_cache = options; } - nm_assert (i == len); - g_qsort_with_data (options, len, sizeof (options[0]), - nm_utils_named_entry_cmp_with_data, NULL); - - NM_SET_OUT (out_name, options[idx].name); - NM_SET_OUT (out_value, options[idx].value_str); + NM_SET_OUT (out_name, priv->options_idx_cache[idx].name); + NM_SET_OUT (out_value, priv->options_idx_cache[idx].value_str); return TRUE; } @@ -365,6 +370,7 @@ nm_setting_bond_add_option (NMSettingBond *setting, priv = NM_SETTING_BOND_GET_PRIVATE (setting); + nm_clear_g_free (&priv->options_idx_cache); g_hash_table_insert (priv->options, g_strdup (name), g_strdup (value)); if ( !strcmp (name, NM_SETTING_BOND_OPTION_MIIMON) @@ -398,6 +404,7 @@ gboolean nm_setting_bond_remove_option (NMSettingBond *setting, const char *name) { + NMSettingBondPrivate *priv; gboolean found; g_return_val_if_fail (NM_IS_SETTING_BOND (setting), FALSE); @@ -405,7 +412,10 @@ nm_setting_bond_remove_option (NMSettingBond *setting, if (!nm_setting_bond_validate_option (name, NULL)) return FALSE; - found = g_hash_table_remove (NM_SETTING_BOND_GET_PRIVATE (setting)->options, name); + priv = NM_SETTING_BOND_GET_PRIVATE (setting); + + nm_clear_g_free (&priv->options_idx_cache); + found = g_hash_table_remove (priv->options, name); if (found) g_object_notify (G_OBJECT (setting), NM_SETTING_BOND_OPTIONS); return found; @@ -920,6 +930,7 @@ set_property (GObject *object, guint prop_id, switch (prop_id) { case PROP_OPTIONS: + nm_clear_g_free (&priv->options_idx_cache); g_hash_table_unref (priv->options); priv->options = _nm_utils_copy_strdict (g_value_get_boxed (value)); break; @@ -960,6 +971,7 @@ finalize (GObject *object) { NMSettingBondPrivate *priv = NM_SETTING_BOND_GET_PRIVATE (object); + nm_clear_g_free (&priv->options_idx_cache); g_hash_table_destroy (priv->options); G_OBJECT_CLASS (nm_setting_bond_parent_class)->finalize (object);