GNOME Bugzilla – Bug 688284
alternate carrier-detect-handling modes
Last modified: 2013-02-15 18:41:14 UTC
The first commit explains the possibilities. I just realized I haven't done the UI side yet. I'm also not sure how the connection editor would know whether or not to display this option for a given connection type, other than by hardcoding a list... One problem with these patches: it breaks autoconnect on wifi, because nm_device_is_available() actually returns FALSE for an NMDeviceWifi at the point when NM is trying to autoactivate it, because the supplicant state has changed from 'ready' to 'inactive'... I'm not sure what's up there. Maybe NMDeviceWifi's is_available() ought to be allowing more supplicant states than just 'ready'?
Created attachment 228932 [details] [review] core: simplify nm_device_get_best_auto_connection() implementations Filter out non-autoconnect connections in the generic NMDevice method rather than requiring each subclass to do it.
Created attachment 228933 [details] [review] libnm-utils: add NMSettingConnection:carrier-behavior Add a property for specifying how device carrier affects a connection, with four possibilities: ignore: The connection ignores carrier on the device; it can be activated when there is no carrier, and stays activated when carrier is lost. activate: The connection can only be activated when the device has carrier, but it will not be deactivated if the device loses carrier. automatic: The connection can only be activated when the device has carrier, and will be deactivated if the device loses carrier for more than 4 seconds. strict: "automatic" without the 4-second grace period.
Created attachment 228934 [details] [review] core: move carrier-detect NMDeviceState handling into NMDevice Move some duplicated carrier-handling code into NMDevice (which can introspect itself to see if it's a subclass that has carrier). When the device is connected, take its NMCarrierBehavior into account.
Created attachment 228935 [details] [review] core: Allow activating appropriate connections on carrierless devices Add a "need_carrier" argument to nm_device_is_available(), to allow distinguishing between "device is not available", "device is fully available", and "device is available except for not having carrier". Adjust various parts of NMDevice and NMManager to allow for the possibility of activating a connection with NMSettingConnection:carrier-behavior = "ignore" on a device with no carrier.
(In reply to comment #0) > The first commit explains the possibilities. where by "first" i mean "second"
Odd, the supplicant state shouldn't go to inactive unless the supplicant is dead or our internal state handling is borked.
I lie. I think the code in is_available() is wrong. It should be: if (state < NM_SUPPLICANT_INTERFACE_STATE_READY || state > NM_SUPPLICANT_INTERFACE_STATE_COMPLETED) { nm_log_dbg (LOGD_WIFI, "(%s): not available because supplicant interface not ready", nm_device_get_iface (dev)); return FALSE; } We added some states a while ago and I bet is_available() wasn't updated for them.
Review of attachment 228932 [details] [review]: Looks good, feel free to commit any time.
(In reply to comment #7) > I lie. I think the code in is_available() is wrong. It should be: > > if (state < NM_SUPPLICANT_INTERFACE_STATE_READY || state > > NM_SUPPLICANT_INTERFACE_STATE_COMPLETED) { > nm_log_dbg (LOGD_WIFI, "(%s): not available because supplicant > interface not ready", > nm_device_get_iface (dev)); > return FALSE; > } > > We added some states a while ago and I bet is_available() wasn't updated for > them. Pushed the fix for this.
Created attachment 234747 [details] [review] core: move carrier-detect NMDeviceState handling into NMDevice Move some duplicated carrier-handling code into NMDevice (which can introspect itself to see if it's a subclass that has carrier).
Created attachment 234748 [details] [review] libnm-utils: add :carrier-detect properties For settings corresponding to devices that have a :carrier property (ie bond, bridge, infiniband, vlan, and wired), add a :carrier-detect property specifying how that affects the connection: yes: The connection can only be activated when the device has carrier, and will be deactivated if the device loses carrier (for more than 4 seconds). no: The connection ignores carrier on the device; it can be activated when there is no carrier, and stays activated when carrier is lost. on-activate: The connection can only be activated when the device has carrier, but it will not be deactivated if the device loses carrier.
Created attachment 234749 [details] [review] core: Update device activation for :carrier-detect Add a "need_carrier" argument to nm_device_is_available(), to allow distinguishing between "device is not available", "device is fully available", and "device is available except for not having carrier". Adjust various parts of NMDevice and NMManager to allow for the possibility of activating a connection with :carrier-detect = "no" on a device with no carrier, and to avoid auto-disconnecting devices with :carrier-detect = "on-activate".
updated for comments from IRC a while back In libnm-util, I moved the property to the individual hardware NMSettings as discussed, and renamed it from "carrier-behavior" to "carrier-detect", with the values "yes", "no", and "on-activate". (This also gets rid of the "auto"/"strict" split, which we weren't actually sure there was a use case for; "yes"/"no" seemed nicer than "automatic"/"ignore", but with yes/no there's no obvious way to distinguish automatic from strict, so I didn't.) The daemon patches only changed to reflect the libnm-util changes. (And yes, the wifi activation problem went away with your NMDeviceWifi is_available() fix, so it looks like everything works now, although I still need to do the UI parts too...)
*** Bug 613422 has been marked as a duplicate of this bug. ***
Created attachment 235745 [details] [review] core: move carrier-detect NMDeviceState handling into NMDevice Move some duplicated carrier-handling code into NMDevice (which can introspect itself to see if it's a subclass that has carrier). The "mostly ignore carrier" special handling for bridges and bonds is now also handled as part of the NMDevice-level carrier handling.
Created attachment 235746 [details] [review] libnm-utils: add :carrier-detect properties For settings corresponding to devices that have a :carrier property (ie bond, bridge, infiniband, vlan, and wired), add a :carrier-detect property specifying how that affects the connection: yes: The connection can only be activated when the device has carrier, and will be deactivated if the device loses carrier (for more than 4 seconds). no: The connection ignores carrier on the device; it can be activated when there is no carrier, and stays activated when carrier is lost. on-activate: The connection can only be activated when the device has carrier, but it will not be deactivated if the device loses carrier.
Created attachment 235747 [details] [review] core: Update device activation for :carrier-detect Add a "need_carrier" argument to nm_device_is_available(), to allow distinguishing between "device is not available", "device is fully available", and "device is available except for not having carrier". Adjust various parts of NMDevice and NMManager to allow for the possibility of activating a connection with :carrier-detect = "no" on a device with no carrier, and to avoid auto-disconnecting devices with :carrier-detect = "on-activate".
Rebased to master, incorporating dcbw's recent changes to master/slave carrier handling, both of which are now handled inside NMDevice. NMSettingBond and NMSettingBridge no longer have the carrier-detect property, since they always behave as carrier-detect=no now.
Review of attachment 235745 [details] [review]: nm-device.c ------ The s_con stuff in carrier_changed() should go, doesn't look like it's useful. Plus if we have priv->act_request, we can be sure we've got an NMConnection and the connection setting. Maybe add some comments later in that function about why enslaved devices don't react to carrier change events the same way as non-slaves? Obviously because if it's a bond slave we don't take it down just because carrier was lost, because the bond handles that condition internally by using failover or whaveter, but would be nice to say.
Review of attachment 235746 [details] [review]: Is there any value to consolidating the carrier-detect verify() parts into a private _utils_xxx() function? It wouldn't save code but it might guard against bugs later if/when we add more detect methods. Your call.
Review of attachment 235747 [details] [review]: Looks good.
hm... just noticed that these patches leave us in a really inconsistent state with respect to logging... do we want to log all carrier changes, or just the ones that we're going to act on?
(In reply to comment #22) > hm... just noticed that these patches leave us in a really inconsistent state > with respect to logging... do we want to log all carrier changes, or just the > ones that we're going to act on? I prefer only those we act on. You can still get the other logs if you turn on debugging and keep LOGD_PLATFORM in.
Created attachment 235880 [details] [review] core: move carrier-detect NMDeviceState handling into NMDevice ===== remove dead code, and reorganize to log the carrier change if and only if we're going to take some action based on it
Created attachment 235881 [details] [review] libnm-utils: add :carrier-detect properties ===== split out helper function
Created attachment 235882 [details] [review] core: Update device activation for :carrier-detect ===== rebase, fix to log the carrier change iff we act on it
Review of attachment 235880 [details] [review]: Looks good.
Review of attachment 235881 [details] [review]: Looks good.
Review of attachment 235882 [details] [review]: Looks good.
Attachment 235880 [details] pushed as fe307db - core: move carrier-detect NMDeviceState handling into NMDevice Attachment 235881 [details] pushed as 5266e25 - libnm-utils: add :carrier-detect properties Attachment 235882 [details] pushed as feeafb8 - core: Update device activation for :carrier-detect