I had a bridge configured with this line in its ifcfg-br0: DELAY=0 I believe I found this parameter from https://docs.fedoraproject.org/en-US/Fedora/17/html/System_Administrators_Guide/s2-networkscripts-interfaces_network-bridge.html (or if not, some similar reference) after configuring bridging and being annoyed by the 30 second delay on activation before the connection actually became usable. I'm about 99% sure that the DELAY=0 setting 'worked' - as in, caused the connection to be activated rapidly - for some time, but then in the last few months (with F21), it seemed to 'stop working'. Logs confirm the delay went back to exactly 30 seconds - 15 seconds for 'listening', 15 seconds for 'learning'. dcbw traced it out and found the kernel actually has a minimum delay of 2 seconds, and treats a setting of 0 as 'not set', so it seems NM was passing a setting of 0 through to the kernel and the kernel was treating it as 'not set, use default'. Indeed, changing the setting to: DELAY=2 seems to 'work', i.e. it makes connection activation pretty quick, with the specified two second delay. Probably NM should treat a setting of <2 as 2, or find a value to pass through to the kernel which will cause the kernel to pick the minimum delay (to guard against the possibility that it changes). The only thing we didn't figure out is why this stopped 'working' for me relatively recently.
NM notes... DELAY (eg, forward_delay) is only valid when STP is enabled. When STP is disabled, NMSettingBridge::forward_delay should be 0. When STP is enabled, forward_delay must be between 2 and 30 seconds. NMSettingBridge::verify() does not enforce the correct value ranges for STP-related options when STP is enabled, but it should do so. So if forward_delay=0 and STP is enabled, the kernel rejects the attempt to set the forward_delay with -ERANGE and the value is left as the default (15 seconds). Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably should do.
Created attachment 955919 [details] [PATCH] bridge: Properly check range of STP properties (In reply to Dan Williams from comment #1) > NM notes... > > DELAY (eg, forward_delay) is only valid when STP is enabled. When STP is > disabled, NMSettingBridge::forward_delay should be 0. The property defaults to 15. The connection with stp=0 and forward_delay unset would not pass validation. > When STP is enabled, > forward_delay must be between 2 and 30 seconds. > > NMSettingBridge::verify() does not enforce the correct value ranges for > STP-related options when STP is enabled, but it should do so. So if > forward_delay=0 and STP is enabled, the kernel rejects the attempt to set > the forward_delay with -ERANGE and the value is left as the default (15 > seconds). > > Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably > should do. Why? It doesn't look like a right place to do that (would it make sense to check this in the keyfile plugin as well then?) and the NMSettingBridge::verify() should cover that anyway.
BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE? Also since min is unsigned, I think Coverity will yell at us for the "if (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0). Not sure we care about that much though, unless you want to change min/max to gint64. Also I'd change "zero" to "zero_valid" to make it clearer. Overall looks good to me.
(In reply to Dan Williams from comment #3) > BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE? Well, either will work. I'd prefer keeping the STP condition for consistency, but I don't really care. > Also since min is unsigned, I think Coverity will yell at us for the "if > (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0). Not > sure we care about that much though, unless you want to change min/max to > gint64. Hmm, I'd assume Coverity is smarter and don't really know what would look good to it instead. Can I keep it as it is here and address the Coverity issues separately (bug #1173171)? > Also I'd change "zero" to "zero_valid" to make it clearer. Done. > Overall looks good to me.
A forward delay of zero used to work. I think F20 broke it. Also, I noticed that I couldn't set it to zero using brctl, so maybe something changed there too
(In reply to Ferry Huberts from comment #5) > A forward delay of zero used to work. > I think F20 broke it. > Also, I noticed that I couldn't set it to zero using brctl, so maybe > something changed there too It actually never "worked", it was just ignored by NetworkManager and then defaulted to 15 anyway.
I distinctly remember setting it to 0 and seeing it at 0. Maybe that was RHEL6
The patch looks good to me (after the changes).
Seems like the fix went out with last f21 update to a later NM Git snapshot.
(In reply to Ferry Huberts from comment #7) > I distinctly remember setting it to 0 and seeing it at 0. > Maybe that was RHEL6 Now confirmed it: DELAY=0 works fine on RHEL6 If fact, I believe it to be a mistake that it currently can't be set any lower than 2.