From 97d3f1b4b99055fa74af913577794f1ed142416d Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 17 Mar 2020 17:36:06 +0100 Subject: [PATCH] nm-setting-bond: don't take default values into account when comparing options This solves a bug exposed by the following cmds: $ nmcli c add type bond ifname bond0 con-name bond0 $ nmcli c modify bond0 +bond.options miimon=100 $ nmcli -f bond.options c show bond0 bond.options: mode=balance-rr Here we just added the option 'miimon=100', but it doesn't get saved in because nm_settings_connection_set_connection() which is responsible for actually updating the connection compares the new connection with old one and if and only if the 2 are different the update is carried out. The bug is triggered because when comparing, if default values are taken into account, then having 'miimon=100' or not having it it's essentially the same for compare(). While this doesn't cause a bond to have a wrong setting when activated it's wrong from a user experience point of view and thus must be fixed. When this patch is applied, the above commands will give the following results: $ nmcli c add type bond ifname bond0 con-name bond0 $ nmcli c modify bond0 +bond.options miimon=100 $ nmcli -f bond.options c show bond0 bond.options: mode=balance-rr,miimon=100 Fix unit tests and also add a new case covering this bug. https://bugzilla.redhat.com/show_bug.cgi?id=1806549 --- libnm-core/nm-setting-bond.c | 15 ++------------- libnm-core/tests/test-setting.c | 12 +++++++----- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index f53f2c8cf2..652d55487b 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -1033,7 +1033,7 @@ options_equal_asym (NMSettingBond *s_bond, NMSettingCompareFlags flags) { GHashTableIter iter; - const char *key, *value, *value2; + const char *key, *value; g_hash_table_iter_init (&iter, NM_SETTING_BOND_GET_PRIVATE (s_bond)->options); while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) { @@ -1048,18 +1048,7 @@ options_equal_asym (NMSettingBond *s_bond, continue; } - value2 = _bond_get_option (s_bond2, key); - - if (!value2) { - if (nm_streq (key, "num_grat_arp")) - value2 = _bond_get_option (s_bond2, "num_unsol_na"); - else if (nm_streq (key, "num_unsol_na")) - value2 = _bond_get_option (s_bond2, "num_grat_arp"); - } - - if (!value2) - value2 = _bond_get_option_default (s_bond2, key); - if (!nm_streq (value, value2)) + if (!nm_streq0 (value, _bond_get_option (s_bond2, key))) return FALSE; } diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index 71f484bab5..2dc76f1000 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -641,21 +641,23 @@ test_bond_compare (void) ((const char *[]){ "mode", "balance-rr", "miimon", "1", NULL }), ((const char *[]){ "mode", "balance-rr", "miimon", "2", NULL })); - /* ignore default values */ - test_bond_compare_options (TRUE, + test_bond_compare_options (FALSE, ((const char *[]){ "miimon", "1", NULL }), ((const char *[]){ "miimon", "1", "updelay", "0", NULL })); - /* special handling of num_grat_arp, num_unsol_na */ test_bond_compare_options (FALSE, ((const char *[]){ "num_grat_arp", "2", NULL }), ((const char *[]){ "num_grat_arp", "1", NULL })); - test_bond_compare_options (TRUE, + test_bond_compare_options (FALSE, ((const char *[]){ "num_grat_arp", "3", NULL }), ((const char *[]){ "num_unsol_na", "3", NULL })); - test_bond_compare_options (TRUE, + test_bond_compare_options (FALSE, ((const char *[]){ "num_grat_arp", "4", NULL }), ((const char *[]){ "num_unsol_na", "4", "num_grat_arp", "4", NULL })); + + test_bond_compare_options (FALSE, + ((const char *[]){ "mode", "balance-rr", "miimon", "100", NULL }), + ((const char *[]){ "mode", "balance-rr", NULL })); } static void