From fb191fc282395e30df6c2c4f332ba4e8b2e5ff2a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 22 Nov 2017 13:55:53 +0100 Subject: [PATCH] ifcfg-rh: use distinct variables for bridge and wired mac address Currently both bridge.mac-address and ethernet.cloned-mac-address get written to the same MACADDR ifcfg-rh variable; the ethernet property wins if both are present. When one property is set and the connection is saved (and thus reread) both properties are populated with the same value. This is wrong because, even if the properties have the same meaning, the setting plugin should not read something different from what was written. Also consider that after the following steps: $ nmcli con mod c ethernet.cloned-mac-address 00:11:22:33:44:55 $ nmcli con mod c ethernet.cloned-mac-address "" the connection will still have the new mac address set in the bridge.mac-address property, which is certainly unexpected. In general, mapping multiple properties to the same variable is harmful and must be avoided. Therefore, let's use a different variable for bridge.mac-address. This changes behavior, but not so much: - connections that have MACADDR set will behave as before; the only difference will be that the MAC will be present in the wired setting instead of the bridge one; - initscripts compatibility is not relevant because MACADDR for bridges was a NM extension; - if someone creates a new connection and sets bridge.mac-address NM will set the BRIDGE_MACADDR property instead of MACADDR. But this shouldn't be a big concern as bridge.mac-address is documented as deprecated and should not be used for new connections. https://bugzilla.redhat.com/show_bug.cgi?id=1516659 --- libnm-core/nm-setting-bridge.c | 4 ++-- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 2 +- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 2 +- src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 5 ++++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/libnm-core/nm-setting-bridge.c b/libnm-core/nm-setting-bridge.c index 7f944bc8e0..187d71d3fe 100644 --- a/libnm-core/nm-setting-bridge.c +++ b/libnm-core/nm-setting-bridge.c @@ -445,10 +445,10 @@ nm_setting_bridge_class_init (NMSettingBridgeClass *setting_class) * ---end--- * ---ifcfg-rh--- * property: mac-address - * variable: MACADDR(+) + * variable: BRIDGE_MACADDR(+) * description: MAC address of the bridge. Note that this requires a recent * kernel support, originally introduced in 3.15 upstream kernel) - * MACADDR for bridges is an NM extension. + * BRIDGE_MACADDR for bridges is an NM extension. * ---end--- */ g_object_class_install_property diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 18d07c117d..bdafbd4261 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4750,7 +4750,7 @@ make_bridge_setting (shvarFile *ifcfg, s_bridge = NM_SETTING_BRIDGE (nm_setting_bridge_new ()); - value = svGetValueStr_cp (ifcfg, "MACADDR"); + value = svGetValueStr_cp (ifcfg, "BRIDGE_MACADDR"); if (value) { value = g_strstrip (value); g_object_set (s_bridge, NM_SETTING_BRIDGE_MAC_ADDRESS, value, NULL); 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 c277cb2ad5..d5cbcd3555 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1408,7 +1408,7 @@ write_bridge_setting (NMConnection *connection, shvarFile *ifcfg, gboolean *wire svUnsetValue (ifcfg, "DELAY"); mac = nm_setting_bridge_get_mac_address (s_bridge); - svSetValueStr (ifcfg, "MACADDR", mac); + svSetValueStr (ifcfg, "BRIDGE_MACADDR", mac); /* Bridge options */ opts = g_string_sized_new (32); 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 e8864d2d6f..7df0c4efad 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -7403,6 +7403,7 @@ test_read_bridge_main (void) { NMConnection *connection; NMSettingBridge *s_bridge; + NMSettingWired *s_wired; const char *mac; char expected_mac_address[ETH_ALEN] = { 0x00, 0x16, 0x41, 0x11, 0x22, 0x33 }; @@ -7425,7 +7426,9 @@ test_read_bridge_main (void) g_assert (!nm_setting_bridge_get_multicast_snooping (s_bridge)); /* MAC address */ - mac = nm_setting_bridge_get_mac_address (s_bridge); + s_wired = nm_connection_get_setting_wired (connection); + g_assert (s_wired); + mac = nm_setting_wired_get_cloned_mac_address (s_wired); g_assert (mac); g_assert (nm_utils_hwaddr_matches (mac, -1, expected_mac_address, ETH_ALEN));