> core: don't fail if bond slave is already a slave of the requested master

NMPlatform will provide nm_platform_link_get_master().

> core: check if slaves are really ready before proceeding with IPv4 config

You are creating two identical functions that could be shared (e.g. as a private/protected method of nm-device). This is how all masterslave functions were done in my own bridging/bonding patches.

> bridge: update log domains for LOGD_BRIDGE

Do we need a special log domain for bridges?

> bond: wait for carrier before starting IP configuration

> bridge: wait for carrier before starting IP configuration

Again, can't the implementation be (at least mostly) shared?

> bond: device availability shouldn't depend on carrier state

> bridge: device availability shouldn't depend on carrier state

Same as above.

(In reply to comment #1)
> > core: don't fail if bond slave is already a slave of the requested master
> 
> NMPlatform will provide nm_platform_link_get_master().

Good to know, but this stuff is targeted for 0.9.8 before we're converting over to the platform.

> > core: check if slaves are really ready before proceeding with IPv4 config
> 
> You are creating two identical functions that could be shared (e.g. as a
> private/protected method of nm-device). This is how all masterslave functions
> were done in my own bridging/bonding patches.

Right, except that bridge/bond slaves may work differently, and I don't want to make a generic function for this until I know for sure that we don't have behavior that's separate between bridges and bonds.  Plus the NMDevice core code is shared for all devices, and I'm not confident that anything else that uses slaves will need this same code.

> > bridge: update log domains for LOGD_BRIDGE
> 
> Do we need a special log domain for bridges?

Yeah, we've got one for BOND and VLAN, makes sense for bridge too.

> > bond: wait for carrier before starting IP configuration
> 
> > bridge: wait for carrier before starting IP configuration
> 
> Again, can't the implementation be (at least mostly) shared?

Again, I'm not 100% sure yet that we want exactly the same behavior.  I decided to keep the code separate for now and the consolidate it later when we  know that.

> > bond: device availability shouldn't depend on carrier state
> 
> > bridge: device availability shouldn't depend on carrier state
> 
> Same as above.

Yeah, same comment here too.  It could be consolidated, but I'm not comfortable doing that yet when we may just need to split it back up again in the future if we find out that bonding has different needs than bridging.  For example, Dan Winships idea about "upstream" ports.  With a bond you *know* they are connected to the same network.  But with a bridge, some ports are downstream (eg connected to your VM) and some are upstream with connectivity (eg, your physical device).  If we decide to handle these differently in the future, we'll need different slave checking behavior between bridge and bond.  The carrier state handling may be different here too as we wouldn't care about the carrier state of downstream ports before starting IP config.

> Right, except that bridge/bond slaves may work differently, and I don't want to
> make a generic function for this until I know for sure that we don't have
> behavior that's separate between bridges and bonds.  Plus the NMDevice core
> code is shared for all devices, and I'm not confident that anything else that
> uses slaves will need this same code.

You can always create a specific version for the other interface and it can even use the generic version and just add some actions.

> Again, I'm not 100% sure yet that we want exactly the same behavior.  I decided
> to keep the code separate for now and the consolidate it later when we  know
> that.

It seems to be the exact opposite order than one would prefer for future code maintainence.

> Yeah, same comment here too.  It could be consolidated, but I'm not comfortable
> doing that yet when we may just need to split it back up again in the future if
> we find out that bonding has different needs than bridging.

This is exactly what genericity and specialization is good for. It is possible to create a generic implementation and just override specific methods in the derived classes.

> For example, Dan
> Winships idea about "upstream" ports.  With a bond you *know* they are
> connected to the same network.  But with a bridge, some ports are downstream
> (eg connected to your VM) and some are upstream with connectivity (eg, your
> physical device).  If we decide to handle these differently in the future,
> we'll need different slave checking behavior between bridge and bond.

Looks like an exact point for possible specialization in the future.

> The
> carrier state handling may be different here too as we wouldn't care about the
> carrier state of downstream ports before starting IP config.

Same here.

I didn't test the branch in a running system, just read with my eyes. Apart from excessive code duplication, it looks good.

The problem with the generalization here is that we dont' have one base class that only Bridge and Bond are subclasses of.  We either make this general in NMDeviceWired and then *block* the new behavior explicitly in NMDeviceEthernet and NMDeviceInfiniband, or we keep it out of the superclass and implement it in the subclasses.  I'll see if there's opportunity to consolidate this at all in a way that doesn't look ugly.

(In reply to comment #4)
> The problem with the generalization here is that we dont' have one base class
> that only Bridge and Bond are subclasses of.  We either make this general in
> NMDeviceWired and then *block* the new behavior explicitly in NMDeviceEthernet
> and NMDeviceInfiniband, or we keep it out of the superclass and implement it in
> the subclasses.  I'll see if there's opportunity to consolidate this at all in
> a way that doesn't look ugly.

The master/slave functions can be triggered by marking an interface as a master interface. AFAIK this is how it works in kernel.

(In reply to comment #4)
> The problem with the generalization here is that we dont' have one base class
> that only Bridge and Bond are subclasses of.  We either make this general in
> NMDeviceWired and then *block* the new behavior explicitly in NMDeviceEthernet
> and NMDeviceInfiniband, or we keep it out of the superclass and implement it in
> the subclasses.  I'll see if there's opportunity to consolidate this at all in
> a way that doesn't look ugly.

Well, also, bonds, bridges, and vlans shouldn't be subclasses of NMDeviceWired anyway, since they can all apply to wifi too.

> core: ignore carrier changes on enslaved interfaces

Hm... so how will this interact with the carrier-detect changes (which I have to get back to...)? Would we just say that "automatic" has a different meaning with slave devices than it does with other devices?

The carrier-detect stuff has an option for "don't deactivate when carrier is lost (but still require carrier to activate in the first place)", which is what this patch does for slaves. So we could skip this patch, and instead just make sure that the connection editor sets all new slave connections to that mode by default.

(Some of the same questions apply to the later changes to bond master carrier handling too.)



> core: preserve errno in nm_utils_do_sysctl()
    
>-	if (nwrote != len) {
>+	if (nwrote != len && saved_errno != EEXIST) {
> 		nm_log_warn (LOGD_CORE, "sysctl: failed to set '%s' to '%s': (%d) %s",

Bad cut+paste? There's no reason to treat EEXIST specially here. (I don't even think it's possible to get EEXIST here...)



> bond: set slave IFF_UP after releasing it

This sounds like a kernel bug... Should we be getting that fixed?

Also, is it actually guaranteed that the device was IFF_UP before we released it?



> bond: wait for carrier before starting IP configuration

Is it possible for a device to be finalized while it has an outstanding ip_retry idle? Or does the g_source_remove() in device_state_changed() prevent that from happening?

>+	/* SLAAC, DHCP, and Link-Local depend on connectivity (and thus slaves)
>+	 * to complete addressing.  SLAAC and DHCP obviously need a peer to
>+	 * provide a prefix, while Link-Local must perform DAD on the local link.
>+	 */

Link-local is no different from full connectivity. Both need to do DAD. But it looks like DAD gets re-run on a NETDEV_UP or NETDEV_CHANGE event... does that imply it gets re-run when carrier changes? If so, you don't need to include link-local here. (And if not, then you can get rid of the whole function, because IPv6 configuration would always require slaves in that case.)



> bridge: wait for carrier before starting IP configuration

didn't read this one; assuming it's the same as the previous patch



> bridge: device availability shouldn't depend on carrier state

the commit message ends in the middle of a sentence

(In reply to comment #7)
> > core: ignore carrier changes on enslaved interfaces
> 
> Hm... so how will this interact with the carrier-detect changes (which I have
> to get back to...)? Would we just say that "automatic" has a different meaning
> with slave devices than it does with other devices?
> 
> The carrier-detect stuff has an option for "don't deactivate when carrier is
> lost (but still require carrier to activate in the first place)", which is what
> this patch does for slaves. So we could skip this patch, and instead just make
> sure that the connection editor sets all new slave connections to that mode by
> default.
> 
> (Some of the same questions apply to the later changes to bond master carrier
> handling too.)

I don't think we care about single slave changes.  We do care about aggregate slave carrier changes.  The kernel already handles generating the master carrier from the slave carriers, so I do think your carrier behavior patches should be used with the master device.  But not for slaves in isolation.

I don't think "require carrier to activate" makes sense for slaves, especially in the case of bonding, because the carrier is expected to be off (or drop) in some situations on slaves.  I'm hard-pressed to imagine a use-case for a bond where the slaves would only be added when their carrier was on, as that would pretty much defeat the purpose of load-balancing or fail-over.

If we deactivate a slave, then it's a candidate for auto-activation, and it would be fairly unexpected to have a slave removed from a bond due to carrier loss, just to have it re-activated with a non-bond-slave connection.

> > core: preserve errno in nm_utils_do_sysctl()
> 
> >-	if (nwrote != len) {
> >+	if (nwrote != len && saved_errno != EEXIST) {
> > 		nm_log_warn (LOGD_CORE, "sysctl: failed to set '%s' to '%s': (%d) %s",
> 
> Bad cut+paste? There's no reason to treat EEXIST specially here. (I don't even
> think it's possible to get EEXIST here...)

Actually that was intentional.  We can get all sorts of errors from sysfs attributes, including EEXIST.  The reason this was added was that on RHEL6 (but no reason it couldn't happen in recent kernels) something adds bond0 underneath NM, before NM can add it.  Since NM was using do_sysctl() to add bonding interfaces via /sys/class/net/bonding_masters, and since that file already had bond0, we'd get an EEXIST.

Basically, the point here is to suppress the log message when in fact there was really no error.  If we try to set a value that already exists, I don't think we should warn about it, since that's a success condition.

> > bond: set slave IFF_UP after releasing it
> 
> This sounds like a kernel bug... Should we be getting that fixed?

We probably should investigate it, yes.  But we still need to work around it in the code.

> Also, is it actually guaranteed that the device was IFF_UP before we released
> it?

No, not guaranteed, but we always keep devices IFF_UP because only then do we get carrier events.  The only case the slave could be down when it's released is if somebody/something *manually* set it down underneath NM, and we don't care much about that.

> > bond: wait for carrier before starting IP configuration
> 
> Is it possible for a device to be finalized while it has an outstanding
> ip_retry idle? Or does the g_source_remove() in device_state_changed() prevent
> that from happening?

That's the idea; I'd originally had one in dispose/finalize, but when the device gets destroyed it'll move to UNMANAGED state first, so we'll cancel the idle from device_state_changed().

Unless we're quitting and leaving the interface up, but then we're quitting so the timeout/idle will never get called anywy :)

> >+	/* SLAAC, DHCP, and Link-Local depend on connectivity (and thus slaves)
> >+	 * to complete addressing.  SLAAC and DHCP obviously need a peer to
> >+	 * provide a prefix, while Link-Local must perform DAD on the local link.
> >+	 */
> 
> Link-local is no different from full connectivity. Both need to do DAD. But it
> looks like DAD gets re-run on a NETDEV_UP or NETDEV_CHANGE event... does that
> imply it gets re-run when carrier changes? If so, you don't need to include
> link-local here. (And if not, then you can get rid of the whole function,
> because IPv6 configuration would always require slaves in that case.)

NETDEV_UP is emitted in response to dev_open(), which is called when setting IFF_UP.  So not carrier.  Carrier changes (which are IFF_LOWER_UP) might trigger CHANGE but we'd have to look at that.

I don't think we can get rid of it for static addressing though, sicne presumably the user knows what they are doing with static, and since they've chosen to manually manage the addresses, they get what they deserve.  eg, we don't wait for slaves.  This is a tricky area though.

The proposal for this originally came from tgraf, who said that sometimes configurations will start a bridge/bond and assign static address to that so that services can bind to the device (eg apache, etc) before slaves are up or available.  We agreed it was limited to cases where you didn't need upstream connectivity.

> > bridge: wait for carrier before starting IP configuration
> 
> didn't read this one; assuming it's the same as the previous patch

It is.

> > bridge: device availability shouldn't depend on carrier state
> 
> the commit message ends in the middle of a sentence

oops :)

(In reply to comment #8)
> I don't think "require carrier to activate" makes sense for slaves

hm... as I understood the patch, that is exactly what you were implementing, because you were removing the carrier checks from the deactivating side, but not modifying the activating side... but I guess slaves don't go through the ordinary auto-activation code, so carrier doesn't actually matter on activation, right? So it's "carrier-detect=no", not "carrier-detect=on-activate"... I guess I'll see how it all looks when I merge this stuff into the carrier branch.

> > Bad cut+paste? There's no reason to treat EEXIST specially here. (I don't even
> > think it's possible to get EEXIST here...)
> 
> Actually that was intentional.  We can get all sorts of errors from sysfs
> attributes, including EEXIST.

ah... wacky. I didn't realize that writing to a file in sysfs could return arbitrary errnos like that.

But, so isn't there the opposite problem as well, where we go to remove the device and it's already been removed, and we get ENOENT or whatever? Do we need to move the logging out of do_sysctl and into the callers, so they can ignore whatever codes they need to?

(In reply to comment #9)
> Do we need
> to move the logging out of do_sysctl and into the callers, so they can ignore
> whatever codes they need to?

and use whatever log domain they want...

(In reply to comment #9)
> (In reply to comment #8)
> > I don't think "require carrier to activate" makes sense for slaves
> 
> hm... as I understood the patch, that is exactly what you were implementing,
> because you were removing the carrier checks from the deactivating side, but
> not modifying the activating side... but I guess slaves don't go through the
> ordinary auto-activation code, so carrier doesn't actually matter on
> activation, right? So it's "carrier-detect=no", not
> "carrier-detect=on-activate"... I guess I'll see how it all looks when I merge
> this stuff into the carrier branch.

Ah yes, you're correct.  The activate behavior has always been carrier here, this patch changes the deactivate trigger.  Do we have carrier behavior items defined for everything we may want to do with bridge/bond slaves?

> > > Bad cut+paste? There's no reason to treat EEXIST specially here. (I don't even
> > > think it's possible to get EEXIST here...)
> > 
> > Actually that was intentional.  We can get all sorts of errors from sysfs
> > attributes, including EEXIST.
> 
> ah... wacky. I didn't realize that writing to a file in sysfs could return
> arbitrary errnos like that.
> 
> But, so isn't there the opposite problem as well, where we go to remove the
> device and it's already been removed, and we get ENOENT or whatever? Do we need
> to move the logging out of do_sysctl and into the callers, so they can ignore
> whatever codes they need to?

We could get ENOENT too I suppose.  The issue was that the places that *don't* want to log on an expected error (like EEXIT and ENOENT) are much fewer than the places that we'd like to know what the error is when it fails.  So I opted to keep the logging in nm_utils_do_sysctl() since breaking the logging out to callers would add a lot more code than just special-casing one or two errors.

I consolidated the duplicate implementations of carrier/slave waiting and a few more fixes from RHEL work.  Branch back up for review.

danw: were you able to reproduce the problem you told me about on Tuesday morning?

"danw: dcbw: oh, i don't know if this is that, but i noticed this morning that bonds only work if you up the slaves *after* upping the bond, not if the slaves are already running when you up the bond"

Hah; looking at it again, perhaps you mean "activate the slave connection first, then bond connection"?  (I was thinking by "up" you meant IFF_UP).

Here's my attempt at that using "nmcli con up uuid <slave uuid>":

NetworkManager[20011]: <info> Activation (bond0) starting connection 'Bond bond0'
NetworkManager[20011]: <info> (bond0): device state change: disconnected -> prepare (reason 'none') [30 40 0]
NetworkManager[20011]: <info> Activation (bond0) Stage 1 of 5 (Device Prepare) scheduled...
NetworkManager[20011]: <info> Activation (eth0) starting connection 'System bondslave0'
NetworkManager[20011]: <info> (eth0): device state change: disconnected -> prepare (reason 'none') [30 40 0]
NetworkManager[20011]: <info> Activation (eth0) Stage 1 of 5 (Device Prepare) scheduled...
NetworkManager[20011]: <info> Activation (bond0) Stage 1 of 5 (Device Prepare) started...
NetworkManager[20011]: <info> (bond0): taking down device.
NetworkManager[20011]: <info> (bond0): bringing up device.
NetworkManager[20011]: <info> Activation (bond0) Stage 2 of 5 (Device Configure) scheduled...
NetworkManager[20011]: <info> Activation (bond0) Stage 1 of 5 (Device Prepare) complete.
NetworkManager[20011]: <info> Activation (eth0) Stage 1 of 5 (Device Prepare) started...
NetworkManager[20011]: <info> Activation (eth0) Stage 2 of 5 (Device Configure) scheduled...
NetworkManager[20011]: <info> Activation (eth0) Stage 1 of 5 (Device Prepare) complete.
NetworkManager[20011]: <info> Activation (bond0) Stage 2 of 5 (Device Configure) starting...
NetworkManager[20011]: <info> (bond0): device state change: prepare -> config (reason 'none') [40 50 0]
NetworkManager[20011]: <info> Activation (bond0) Stage 2 of 5 (Device Configure) successful.
NetworkManager[20011]: <info> Activation (bond0) Stage 3 of 5 (IP Configure Start) scheduled.
NetworkManager[20011]: <info> Activation (bond0) Stage 2 of 5 (Device Configure) complete.
NetworkManager[20011]: <info> Activation (eth0) Stage 2 of 5 (Device Configure) starting...
NetworkManager[20011]: <info> (eth0): device state change: prepare -> config (reason 'none') [40 50 0]
NetworkManager[20011]: <info> Activation (eth0) Stage 2 of 5 (Device Configure) successful.
NetworkManager[20011]: <info> Activation (eth0) Stage 3 of 5 (IP Configure Start) scheduled.
NetworkManager[20011]: <info> Activation (eth0) Stage 2 of 5 (Device Configure) complete.
NetworkManager[20011]: <info> Activation (bond0) Stage 3 of 5 (IP Configure Start) started...
NetworkManager[20011]: <info> (bond0): device state change: config -> ip-config (reason 'none') [50 70 0]
NetworkManager[20011]: <info> (bond0): IPv4 config waiting until carrier is on
NetworkManager[20011]: <info> (bond0): IPv6 config waiting until carrier is on
NetworkManager[20011]: <info> Activation (bond0) Stage 3 of 5 (IP Configure Start) complete.
NetworkManager[20011]: <info> Activation (eth0) Stage 3 of 5 (IP Configure Start) started...
NetworkManager[20011]: <info> (eth0): device state change: config -> ip-config (reason 'none') [50 70 0]
NetworkManager[20011]: <info> (eth0): taking down device.
NetworkManager[20011]: <info> (bond0): enslaved bond slave eth0
NetworkManager[20011]: <info> Activation (eth0) connection 'System bondslave0' enslaved, continuing activation
NetworkManager[20011]: <info> (bond0): IPv4 config waiting until carrier is on
NetworkManager[20011]: <info> (bond0): IPv6 config waiting until carrier is on
NetworkManager[20011]: <info> Activation (eth0) Stage 3 of 5 (IP Configure Start) complete.
NetworkManager[20011]: <info> (eth0): carrier now OFF (device state 70, deferring action for 4 seconds)
NetworkManager[20011]: <info> (eth0): device state change: ip-config -> secondaries (reason 'none') [70 90 0]
NetworkManager[20011]: <info> (eth0): device state change: secondaries -> activated (reason 'none') [90 100 0]
NetworkManager[20011]: <info> Activation (eth0) successful, device activated.
NetworkManager[20011]: <info> (eth0): carrier now ON (device state 100)
NetworkManager[20011]: <info> (bond0): carrier now ON (device state 70)
NetworkManager[20011]: <info> Activation (bond0) Beginning DHCPv4 transaction (timeout in 45 seconds)
NetworkManager[20011]: <info> dhclient started with pid 21626
NetworkManager[20011]: <info> Activation (bond0) Beginning IP6 addrconf.
Internet Systems Consortium DHCP Client 4.2.4-P2
Copyright 2004-2012 Internet Systems Consortium.
All rights reserved.
For info, please visit https://www.isc.org/software/dhcp/

NetworkManager[20011]: <info> (bond0): DHCPv4 state changed nbi -> preinit
Listening on LPF/bond0/00:23:5a:47:1f:71
Sending on   LPF/bond0/00:23:5a:47:1f:71
Sending on   Socket/fallback
DHCPREQUEST on bond0 to 255.255.255.255 port 67 (xid=0x3ef8285b)
DHCPACK from 192.168.1.1 (xid=0x3ef8285b)
bound to 192.168.1.130 -- renewal in 280 seconds.
NetworkManager[20011]: <info> (bond0): DHCPv4 state changed preinit -> reboot
NetworkManager[20011]: <info>   address 192.168.1.130
NetworkManager[20011]: <info>   prefix 24 (255.255.255.0)
NetworkManager[20011]: <info>   gateway 192.168.1.1
NetworkManager[20011]: <info>   nameserver '192.168.1.1'
NetworkManager[20011]: <info> Activation (bond0) Stage 5 of 5 (IPv4 Configure Commit) scheduled...
NetworkManager[20011]: <info> Activation (bond0) Stage 5 of 5 (IPv4 Commit) started...
NetworkManager[20011]: <info> (bond0): device state change: ip-config -> secondaries (reason 'none') [70 90 0]
NetworkManager[20011]: <info> Activation (bond0) Stage 5 of 5 (IPv4 Commit) complete.
NetworkManager[20011]: <info> (bond0): device state change: secondaries -> activated (reason 'none') [90 100 0]
NetworkManager[20011]: <info> Policy set 'Bond bond0' (bond0) as default for IPv4 routing and DNS.
NetworkManager[20011]: <info> Activation (bond0) successful, device activated.

Which works for me...  Is that not the case for you?

(In reply to comment #13)
> danw: were you able to reproduce the problem you told me about on Tuesday
> morning?
> 
> "danw: dcbw: oh, i don't know if this is that, but i noticed this morning that
> bonds only work if you up the slaves *after* upping the bond, not if the slaves
> are already running when you up the bond"
> 
> Hah; looking at it again, perhaps you mean "activate the slave connection
> first, then bond connection"?  (I was thinking by "up" you meant IFF_UP).

Yes, that's what I meant. Anyway, this was mostly just me being confused by the way bonds work again. I did:

  nmcli con up id 'bond0 slave1'
  nmcli con up id 'Bond connection 1'

and the second command hangs. Of course, the bond connection was already up at that point and I didn't have to type that command, but probably NM should have noticed and returned either an error or immediate success...

(So this means that to bring a bond up, you bring up any of its slaves, but to bring it down, you have to bring down the bond itself, not the slaves...)

> core: expose IP4 wait state to subclasses

>+nm_netlink_monitor_request_bridge_info (NMNetlinkMonitor *self, GError **error)

>+	/* FIXME: nl_rtgen_request() gets the return value screwed up with
>+	 * libnl-1.1; revisit this and return a proper error when we port to
>+	 * a later libnl.
>+	 */

libnl-1 support has been removed



> core: let subclasses find slaves by ifindex

>+ * Returns: the slave with the given @ifindex of which @device is the master,
>+ *   or %NULL if no device with @ifinidex is a slave of @device.

typo ("ifinidex")



> bridge: don't start auto IP config until at least one slave is forwarding

the whole "PROT" vs "port" thing here is nasty, but there's not much you can do...

>+	if (nlmsg_parse (hdr, sizeof (*ifi), tb, IFLA_MAX, link_policy) < 0) {
>+		nm_log_dbg (LOGD_BRIDGE, "ignoring invalid newlink netlink message "
>+		            "while parsing PROTINFO attribute");

nm-ip6-manager prints the same error (obviously, since you apparently copied+pasted this from there...) so it would get logged twice... maybe NMNetlinkMonitor should do this itself...



>wired: wait for carrier on master devices before proceeding with IP config

> will just take longer or fail.  Recent kernels have cleaned up carrier
> handling on bridge/bond devices and the carrier is now a reliable
> indication of whether or not the master device is ready to send/receive
> traffic.

doesn't that imply the BR_STATE_FORWARDING patch is unnecessary?

No other changes except the most recent patch, where I took Pavel's suggestion and made NM only manage bridges it creates.  I know the solution isn't foolproof, especially if NM crashes (whereupon restart it will ignore bridges it created because the nm-bridges file didn't get written), but it should be sufficient for the next 3 months until we can handle bridges non-destructively.

(In reply to comment #15)
> >wired: wait for carrier on master devices before proceeding with IP config
> 
> > will just take longer or fail.  Recent kernels have cleaned up carrier
> > handling on bridge/bond devices and the carrier is now a reliable
> > indication of whether or not the master device is ready to send/receive
> > traffic.
> 
> doesn't that imply the BR_STATE_FORWARDING patch is unnecessary?

You know, it might.  I'll double-check that.  From a quick review of the kernel code in net/bridge/br_stp.c::br_port_state_selection() it appears that the bridge's carrier will be turned on whenever at least one port is BR_STATE_FORWARDING.  Would be quite nice to kill that port state code.

(In reply to comment #17)
> (In reply to comment #15)
> > >wired: wait for carrier on master devices before proceeding with IP config
> > 
> > > will just take longer or fail.  Recent kernels have cleaned up carrier
> > > handling on bridge/bond devices and the carrier is now a reliable
> > > indication of whether or not the master device is ready to send/receive
> > > traffic.
> > 
> > doesn't that imply the BR_STATE_FORWARDING patch is unnecessary?
> 
> You know, it might.  I'll double-check that.  From a quick review of the kernel
> code in net/bridge/br_stp.c::br_port_state_selection() it appears that the
> bridge's carrier will be turned on whenever at least one port is
> BR_STATE_FORWARDING.  Would be quite nice to kill that port state code.

Yes, carrier appears to be an accurate flag for the bridge being usable on recent kernels.  I don't believe it was on RHEL6 where these patches originated.  Good catch, I'll drop that patch.

Ready for review again, fixed up from comments on IRC.  Only changes are the dropped bridge state patch and the last patch for selective bridge management.

(12:19:49 PM) danw: dcbw: yes, good to push

Merged.

Cool to have this in the master branch!