GNOME Bugzilla – Bug 755182
[review] Wake-on-LAN default value alters the existing value [bg/wol-add-ignore-option-bgo755182]
Last modified: 2015-10-20 16:39:18 UTC
In 1.0.6, setting Wake-on-LAN to "default" (i.e. not setting WoL it at all) is equivalent to "wol d", disable Wake-on-LAN. If the wol value was previously set e.g. via udev rules, NetworkManager effectively breaks that configuration, instead of leaving it untouched. So, in my opinion, the "default" value for Wake-on-LAN should not alter the existing wol value.
(In reply to Jacobo Pantoja from comment #0) > So, in my opinion, the "default" value for Wake-on-LAN should not alter the > existing wol value. I agree, this is probably the most reasonable behavior. Pushed branch bg/wol-add-ignore-option-bgo755182 which adds a new 'ignore' wake-on-lan option and makes it the default.
- if (NM_FLAGS_HAS (wol, NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT) && - NM_FLAGS_ANY (wol, NM_SETTING_WIRED_WAKE_ON_LAN_ALL)) { + if ( NM_FLAGS_HAS (wol, NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT) + && wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT) { why did you change this? OK, the usage of ALL here would be confusing (see [1]). Alternatively: if ( NM_FLAGS_HAS (wol, NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT) && && NM_FLAGS_ANY (wol, ~NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)) { [1] Actually, I find it confusing that NM_SETTING_WIRED_WAKE_ON_LAN_ALL does not contain DEFAULT|IGNORE... thus I agree with that change above. Maybe we should have ALL really be "all" flags? NM_SETTING_WIRED_WAKE_ON_LAN_ALL = (((_NM_SETTING_WIRED_WAKE_ON_LAN_LAST - 1) << 1) - 1 - NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT - NM_SETTING_WIRED_WAKE_ON_LAN_IGNORE) similarly, wol = _nm_utils_ascii_str_to_int64 (value, 10, NM_SETTING_WIRED_WAKE_ON_LAN_NONE, - NM_SETTING_WIRED_WAKE_ON_LAN_ALL, + _NM_SETTING_WIRED_WAKE_ON_LAN_LAST - 1, NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT); this usage of "LAST - 1" is seemingly correct, but only accidentally because LAST==IGNORE and IGNORE can only be set alone. I find that confusing for a flag-typed argument (toghether with ALL not being all). Imagine you set the default-configuration to "ethernet.wake-on-lan=1025"/ Preferably, parsing the default value has the same strict validation as nm-setting-wired:verify() has -- thus forbidding nonsense like 0x1025 ("IGNORE|DEFAULT"). Changed the implementation of parse_ethtool_options in a fixup!.
(In reply to Thomas Haller from comment #2) > [1] Actually, I find it confusing that NM_SETTING_WIRED_WAKE_ON_LAN_ALL does > not contain DEFAULT|IGNORE... thus I agree with that change above. Maybe we > should have ALL really be "all" flags? ALL has the meaning of "wake on ALL events", which perhaps is a bit misleading, but we need a value representing all 'real' wol flags. Other flags like default and ignore are used only to set NM behavior w.r.t wake-on-lan. At the moment I don't have better ideas about how to encode all the possibilities (and also, this was released in 1.0.6 and thus the values can't be changed now, I think). Pushed a fixup which reorders a bit the enum values and simplifies the checks. > Imagine you set the default-configuration to "ethernet.wake-on-lan=1025"/ > Preferably, parsing the default value has the same strict validation as > nm-setting-wired:verify() has -- thus forbidding nonsense like 0x1025 > ("IGNORE|DEFAULT"). Right, fixed. > Changed the implementation of parse_ethtool_options in a fixup!. Looks good, thanks.
I like your last change... pushed another fixup. IMO it's good!!
Merged to master: b088898814dc56e0e12771be66b9d201f1acd246
Merged to nm-1-0 as well with 2 additional commits by thaller: 7cf30fe47ee4 wake-on-lan: add option to keep existing settings efa303ed82c9 ifcfg-rh: improve parsing of Wake-on-LAN options 77f43defbe6d ifcfg-rh: remove unused variable in parse_ethtool_option() 2631174d4eeb ifcfg-rh: remove another unused variable in parse_ethtool_option()