This consists of two action-items:


1.) [platform] pass on the MAC address as netlink attribute IFLA_ADDRESS when creating a bridge



2.) [setting] make the mac address configurable via a setting like NM_SETTING_BRIDGE_CLONED_MAC_ADDRESS. The biggest question here is the name. 
 
NM_SETTING_BRIDGE_CLONED_MAC_ADDRESS is similar to NM_SETTING_WIRED|WIRELESS_CLONED_MAC_ADDRESS, but not fully -- because the latter change the MAC address of a real device, while the new property should set the mac address of the (software) bridge.

An alternative would be NM_SETTING_BRIDGE_MAC_ADDRESS, but also might not be the same as other NM_SETTING_*_MAC_ADDRESS properties, where the property is used to "match" the device for a connection.

Also consider, that later we might need a similar property for other software devices such as veth or tap.

Branch for review: th/bgo729844_set_bridge_mac_address

(I did not test it yet)

> libnm-util: add property NM_SETTING_BRIDGE_MAC_ADDRESS to NMSettingBridge
missing Since: 0.9.10 in nm_setting_bridge_get_mac_address()

> core: match the NMSettingBridge:mac-address in NMDeviceBridge:check_connection_compatible()
+		if (!platform_address ||
+		    platform_address_len != mac_address->len ||
+		    memcmp (mac_address->data, platform_address, 
platform_address_len) != 0) {
We usually put || in front of conditions.

I have pushed "cli: support new bridge.mac-address property" on top of the branch.

(In reply to comment #3)
> > libnm-util: add property NM_SETTING_BRIDGE_MAC_ADDRESS to NMSettingBridge
> missing Since: 0.9.10 in nm_setting_bridge_get_mac_address()
> 
> > core: match the NMSettingBridge:mac-address in NMDeviceBridge:check_connection_compatible()
> +        if (!platform_address ||
> +            platform_address_len != mac_address->len ||
> +            memcmp (mac_address->data, platform_address, 
> platform_address_len) != 0) {
> We usually put || in front of conditions.

Both fixed.


> I have pushed "cli: support new bridge.mac-address property" on top of the
> branch.

Thanks, your commit looks good to me

Added two more commits to the branch:
ifcfg-rh: read/write bridge.mac-address property as MACADDR variable
cli: add optional 'mac' argument for 'nmcli con add type bridge'

I'm not sure whether we should use MACADDR or HWADDR in ifcfg-rh plugin.

(In reply to comment #5)
> Added two more commits to the branch:

Awesome. 



> ifcfg-rh: read/write bridge.mac-address property as MACADDR variable

-         PARSE_WARNING ("%s", (*error)->message);
+         PARSE_WARNING ("%s", error ? (*error)->message : "unknown error reading MACADDR");


-    if (mac) {
+    g_warn_if_fail (!mac || mac->len == 6);
+    if (mac && mac->len == 6) {



-         char *tmp;
-
-         tmp = g_strdup_printf ("%02X:%02X:%02X:%02X:%02X:%02X",
-                                mac->data[0], mac->data[1], mac->data[2],
-                                mac->data[3], mac->data[4], mac->data[5]);
-         svSetValue (ifcfg, "MACADDR", tmp, FALSE);
-         g_free (tmp);
+         char tmp[18];
+
+         g_snprintf (tmp, sizeof (tmp), "%02X:%02X:%02X:%02X:%02X:%02X",
+                     mac->data[0], mac->data[1], mac->data[2],
+                     mac->data[3], mac->data[4], mac->data[5]);
+         svSetValue (ifcfg, "MACADDR", tmp, FALSE);




> cli: add optional 'mac' argument for 'nmcli con add type bridge'

looks good, I added a fixup! for bash-completion

> libnm-util: add property NM_SETTING_BRIDGE_MAC_ADDRESS to NMSettingBridge

+	 * If specified, the mac address of bridge. When creating a new bridge, this mac address
+	 * will be set. When matching an existing (outside NetworkManager created) bridge, this
+	 * mac address must match.

Capitalize MAC address.

> core: match the NMSettingBridge:mac-address in NMDeviceBridge:check_connection_compatible()

Wouldn't nm_device_get_hw_address() work here?  NMDevice calls nm_device_update_hw_address() at various points (construct time, bring_up time) which should ensure that nm_device_get_hw_address() has the right MAC address for the device.  If not, we should fix that.

> core: set NMSettingBridge:mac-address when creating new bridge

We should note in the git commit message that this won't work unless the user's kernel has the specified commit (and what kernel versions work, if we can find that out).

> ifcfg-rh: read/write bridge.mac-address property as MACADDR variable

Could we drop MACADDR into one of the existing bridge testcases to make sure it gets read/written correctly too?  Should be enough to add it to test_read_bridge_main(), and then read/write it from test_write_bridge_main().

Overall looks good!

(In reply to comment #7)
> > libnm-util: add property NM_SETTING_BRIDGE_MAC_ADDRESS to NMSettingBridge
> 
> +     * If specified, the mac address of bridge. When creating a new bridge,
> this mac address
> +     * will be set. When matching an existing (outside NetworkManager created)
> bridge, this
> +     * mac address must match.
> 
> Capitalize MAC address.

Done


> > core: match the NMSettingBridge:mac-address in NMDeviceBridge:check_connection_compatible()
> 
> Wouldn't nm_device_get_hw_address() work here?  NMDevice calls
> nm_device_update_hw_address() at various points (construct time, bring_up time)
> which should ensure that nm_device_get_hw_address() has the right MAC address
> for the device.  If not, we should fix that.

Done


> > core: set NMSettingBridge:mac-address when creating new bridge
> 
> We should note in the git commit message that this won't work unless the user's
> kernel has the specified commit (and what kernel versions work, if we can find
> that out).

Added comment to squash! commit. Is it ok?



> > ifcfg-rh: read/write bridge.mac-address property as MACADDR variable
> 
> Could we drop MACADDR into one of the existing bridge testcases to make sure it
> gets read/written correctly too?  Should be enough to add it to
> test_read_bridge_main(), and then read/write it from test_write_bridge_main().

@Jirka?

(In reply to comment #8)
> 
> > > ifcfg-rh: read/write bridge.mac-address property as MACADDR variable
> > 
> > Could we drop MACADDR into one of the existing bridge testcases to make sure it
> > gets read/written correctly too?  Should be enough to add it to
> > test_read_bridge_main(), and then read/write it from test_write_bridge_main().
> 
> @Jirka?
I've added a fixup! commit.

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > > > ifcfg-rh: read/write bridge.mac-address property as MACADDR variable
> > > 
> > > Could we drop MACADDR into one of the existing bridge testcases to make sure it
> > > gets read/written correctly too?  Should be enough to add it to
> > > test_read_bridge_main(), and then read/write it from test_write_bridge_main().
> > 
> > @Jirka?
> I've added a fixup! commit.

Fixup look good, except:

+\(en MAC address of the bridge (note: this needs kernel >= 3.15-rc5)

I would not mention explicit kernel numbers, because distribution kernels easily could backport this feature, while still having an lower kernel version.

how about:

+\(en MAC address of the bridge (note: this needs a recent kernel feature, originally introduced in 3.15 upstream kernel)

(In reply to comment #10)
> how about:
> 
> +\(en MAC address of the bridge (note: this needs a recent kernel feature,
> originally introduced in 3.15 upstream kernel)

Sounds good. Pushed a fixup on top of the previous fixup.

Branch looks good.

Rebased to master and added a commit
>> keyfile: support NM_SETTING_BRIDGE_MAC_ADDRESS property

Fixes look good, including change to HWADDR instead of MACADDR.

(In reply to comment #14)
> Fixes look good, including change to HWADDR instead of MACADDR.

After consulting with Jirka, I will revert it to MACADDR. The reason is that ifup-eth has the following lines:

First:


# bail out, if the MAC does not fit
if [ -n "${HWADDR}" ]; then
    FOUNDMACADDR=$(get_hwaddr ${REALDEVICE})
    if [ "${FOUNDMACADDR}" != "${HWADDR}" -a "${FOUNDMACADDR}" != "${MACADDR}" ]; then
        net_log $"Device ${DEVICE} has different MAC address than expected, ignoring."
        exit 1
    fi
fi



later:

if [ -n "${MACADDR}" ]; then
   ip link set dev ${DEVICE} address ${MACADDR}
fi




So, for compatibility, the name should be MACADDR.

Ok, sounds good.

Merged to upstream master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=fad5472b343f89ce384c61e6cb40016880b9b70c

*** Bug 731057 has been marked as a duplicate of this bug. ***