Bug 1141417 - Persistent wake on lan across reboot
Summary: Persistent wake on lan across reboot
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: x86_64
OS: Linux
medium
medium
Target Milestone: rc
: ---
Assignee: Beniamino Galvani
QA Contact: Desktop QE
Mark Flitter
URL:
Whiteboard:
: 1203102 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-09-13 04:21 UTC by Arun Tomar
Modified: 2016-02-25 18:12 UTC (History)
12 users (show)

Fixed In Version:
Doc Type: Release Note
Doc Text:
NetworkManager now supports Wake On Lan The nmcli utility now allows "Wake on Lan" to be set on a per device basis.
Clone Of:
Environment:
Last Closed: 2015-11-19 10:57:27 UTC
Target Upstream Version:


Attachments (Terms of Use)
[patch] obsolete work in prograss by danw [danw/wip/ethtool] (12.40 KB, patch)
2016-02-25 18:12 UTC, Thomas Haller
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2015:2315 normal SHIPPED_LIVE Moderate: NetworkManager security, bug fix, and enhancement update 2015-11-19 10:06:58 UTC

Description Arun Tomar 2014-09-13 04:21:48 UTC
Description of problem:
I was to enable and use wake on lan on my RHEL7.0/CENTOS 7.0 system. I've enabled wake on lan from the bios. From internet search, i found that for earlier versions, we had to add ETHTOOL_OPTS="wol g" in the interfaces ifcfg file. but now, network manager controls the networking and even after adding that required option to the file, it's not persistent across reboots. 

Version-Release number of selected component (if applicable):
RHEL7.0/CENTOS7.0


How reproducible:


Steps to Reproduce:
1. Enable wake on lan from bios. 
2. As there is no option for config/enable wake on lan in network manager, it is to be enabled temporarily via ethtool using the command
" ethtool -s enp3s5f0 wol g"

3. For persistent settings, for the earlier version, it was supposed to be added to the ifcfg-< interface> file
ETHTOOL_OPTS="wol g"
4. reboot the system and check the ethtool <interface > output. if in the result you get " Wake-on: g" then all is good and working, if you get " Wake-on: d" that means it's disabled, which is the default. 

Actual results:
[root@phenom ~]# ethtool enp3s5f0
Settings for enp3s5f0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: on (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes



Expected results:
[root@phenom ~]# ethtool enp3s5f0
Settings for enp3s5f0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: on (auto)
        Supports Wake-on: umbg
        Wake-on: g
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes



Additional info:

Comment 2 Dan Winship 2014-09-15 12:34:20 UTC
(In reply to Arun Tomar from comment #0)
> I was to enable and use wake on lan on my RHEL7.0/CENTOS 7.0 system. I've
> enabled wake on lan from the bios. From internet search, i found that for
> earlier versions, we had to add ETHTOOL_OPTS="wol g" in the interfaces ifcfg
> file. but now, network manager controls the networking and even after adding
> that required option to the file, it's not persistent across reboots. 

While we could add support for this to NM, /usr/share/doc/initscripts/sysconfig.txt lists ETHTOOL_OPTS under "Deprecated, but supported", and notes:

      Long term, this should be done by sysadmin-written udev rules.

Though it doesn't give the details of how you'd do that... so I think this is mostly a documentation issue. And maybe also ifup should print a warning telling you to use udev rules if you use ETHTOOL_OPTS?

Comment 4 Dan Williams 2014-12-09 15:24:59 UTC
I could argue that WOL can be network-specific and thus should be per-network configuration in NMSettingEthernet/NMSettingWireless/etc.  So my vote would be that NM can handle it and we make at least a small effort to read/write ETHTOOL_OPTS in ifcfg-rh.

Comment 6 Jirka Klimes 2015-05-22 12:16:49 UTC
*** Bug 1203102 has been marked as a duplicate of this bug. ***

Comment 7 Beniamino Galvani 2015-06-01 09:31:12 UTC
Branch bg/wol-rh1141417 implements Wake-on-LAN settings, if we agree that NM should handle them.

Comment 8 Thomas Haller 2015-06-01 10:05:08 UTC
>> libnm-core: add Wake on LAN properties to NMSettingWired

+    NM_SETTING_WIRED_WAKE_ON_ARP       = (1 << 4),
+    NM_SETTING_WIRED_WAKE_ON_MAGIC     = (1 << 5)
+} NMSettingWiredWakeOnLan;

MAybe the names should be
  NM_SETTING_WIRED_WAKE_ON_LAN_*
so that they match the name of the enum type.


I think there should also be a special flag NM_SETTING_WIRED_WAKE_ON_LAN_NONE=0.


You could add a special flag
  NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT = (1<<0)
and that should be the default for NMSettingWired.


Together with connection defaults, that would allow overwriting the default setting via

[connection.eth0]
ethernet.wake-on-lan=32
ethernet.wake-on-lan-password=magic


That doesn't have to happen now (or ever), but having a distinct default value is important now (to be able to add it later).


swap order of commits:

* ifcfg-rh: add support for Wake on LAN ethtool options
* cli: add support for Wake on LAN settings



>> core: configure Wake on LAN parameters for Ethernet devices

non-platform code should not use ethtool directly. Instead, you should use the utils function from platform. (Yes, we do that already in get_link_speed())

Could you add it this functionality to nm-platform-utils.c after we merge:
  http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=th/platform_refact_caching-bgo747981&id=2db98e996ec3e328a94b3902bc5fd72dda01aff9

Comment 9 Thomas Haller 2015-06-01 10:19:47 UTC
(In reply to Thomas Haller from comment #8)

> >> core: configure Wake on LAN parameters for Ethernet devices
> 
> non-platform code should not use ethtool directly. Instead, you should use
> the utils function from platform. (Yes, we do that already in
> get_link_speed())

I moved get_link_speed() to nm-platform-utils.c, see th/platform_refact_caching-bgo747981 branch.

Comment 10 Beniamino Galvani 2015-06-05 08:55:23 UTC
(In reply to Thomas Haller from comment #8)
> >> libnm-core: add Wake on LAN properties to NMSettingWired
> MAybe the names should be
>   NM_SETTING_WIRED_WAKE_ON_LAN_*
> so that they match the name of the enum type.

Fixed.

> I think there should also be a special flag
> NM_SETTING_WIRED_WAKE_ON_LAN_NONE=0.
> 
> You could add a special flag
>   NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT = (1<<0)
> and that should be the default for NMSettingWired.
> 
> That doesn't have to happen now (or ever), but having a distinct default
> value is important now (to be able to add it later).

I added the NONE and the DEFAULT flags. DEFAULT is not used at the moment because it's not handled in any way; after the connection-defaults branch is merged I'll make it as default and add code in nmcli and core to deal with it.

> swap order of commits:
> 
> * ifcfg-rh: add support for Wake on LAN ethtool options
> * cli: add support for Wake on LAN settings

Ok.

> >> core: configure Wake on LAN parameters for Ethernet devices
>
> Could you add it this functionality to nm-platform-utils.c after we merge:
>  
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=th/
> platform_refact_caching-bgo747981&id=2db98e996ec3e328a94b3902bc5fd72dda01aff9

Sure, for now the code is still in nm-device-ethernet.c but I'll move it once the branch is merged.

Comment 11 Thomas Haller 2015-06-05 10:20:08 UTC

(priv->wol & NM_SETTING_WIRED_WAKE_ON_LAN_MAGIC) == 0

NM_FLAGS_HAS()



+    nm_utils_wake_on_lan_parse_option;
+    nm_utils_wake_on_lan_build_option_string;
I think these functions have a very good place in libnm.

But probably in the future (also with better metadata-support in libnm), we would add much more flags2str and str2flags functions. Maybe lets find a naming scheme for those enum/flags conversions.

I tend to:

+    nm_utils_flags2str_wake_on_lan;
+    nm_utils_str2flags_wake_on_lan;


also, from the signature of the str2flags function, maybe it be better to have it:
  const char *nm_utils_attr2flags_(Enum value, char *buf, gsize buf_len);



Also, IMO it be better to add generic utils function like in libnl: https://github.com/thom311/libnl/blob/a9c789ccd4e24db3b14ffaf3f57ee3e3a57f6fca/lib/route/addr.c#L1077


+    { 'p', NM_SETTING_WIRED_WAKE_ON_LAN_PHY },
+    { 'u', NM_SETTING_WIRED_WAKE_ON_LAN_UNICAST },
+    { 'm', NM_SETTING_WIRED_WAKE_ON_LAN_MULTICAST },

also, I'd make the string values longer and self explanatory.

Comment 12 Beniamino Galvani 2015-06-08 12:48:21 UTC
(In reply to Thomas Haller from comment #11)
> 
> (priv->wol & NM_SETTING_WIRED_WAKE_ON_LAN_MAGIC) == 0
> 
> NM_FLAGS_HAS()

Fixed.

> +    nm_utils_wake_on_lan_parse_option;
> +    nm_utils_wake_on_lan_build_option_string;
> I think these functions have a very good place in libnm.
> 
> But probably in the future (also with better metadata-support in libnm), we
> would add much more flags2str and str2flags functions. Maybe lets find a
> naming scheme for those enum/flags conversions.

Ok, now nm-meta.c string-flags conversions are implemented by generic code in nm-meta.c.
 
> +    { 'p', NM_SETTING_WIRED_WAKE_ON_LAN_PHY },
> +    { 'u', NM_SETTING_WIRED_WAKE_ON_LAN_UNICAST },
> +    { 'm', NM_SETTING_WIRED_WAKE_ON_LAN_MULTICAST },
> 
> also, I'd make the string values longer and self explanatory.

Changed and repushed, thanks.

Comment 13 Thomas Haller 2015-06-08 13:21:13 UTC
(In reply to Beniamino Galvani from comment #12)

> > +    nm_utils_wake_on_lan_parse_option;
> > +    nm_utils_wake_on_lan_build_option_string;
> > I think these functions have a very good place in libnm.
> > 
> > But probably in the future (also with better metadata-support in libnm), we
> > would add much more flags2str and str2flags functions. Maybe lets find a
> > naming scheme for those enum/flags conversions.
> 
> Ok, now nm-meta.c string-flags conversions are implemented by generic code
> in nm-meta.c.

I like it...

I would make

+typedef struct {
+    const char *name;
+    int value;
+} NMMetaFlagDesc;

an internal type.

typedef struct _NMMetaFlagDesc NMMetaFlagDesc;

So that we don't have to commit on the internals right away. For example, later we might want also a NMMetaFlag type for int64 size flags (which would require a nm_meta_flags642str() too).


Also, could we rename "NMMetaFlagDesc" to "NMMetaFlag"? It's really a meta object for the flag-type, not only it's description. Maybe we want to extend it for other applications.


also, I would rename nm_meta_flags2str(), nm_meta_str2flags() to something like nm_meta_flag_*() so that they look like a method on the NMMetaFlag type. Maybe nm_meta_flag_to_str(), nm_meta_flag_from_value().

Comment 14 Beniamino Galvani 2015-06-08 16:07:41 UTC
(In reply to Thomas Haller from comment #13)

Fixed and repushed.

Comment 15 Lubomir Rintel 2015-06-09 14:44:55 UTC
All commits LGTM

Comment 16 Jirka Klimes 2015-06-10 07:06:29 UTC
> libnm-core: add Wake-on-LAN properties to NMSettingWired
*
nm_setting_wired_get_wake_on_lan() misses "Returns:" line like
"Returns: the Wake-on-LAN modes"

*
g_param_spec_uint (NM_SETTING_WIRED_WAKE_ON_LAN,
                    "Wake-on-LAN",
                    "The Wake-on-LAN modes to use. "
                    "Not all devices support all modes.",

Do not put nick and blurb description to g_param_spec_uint(). We removed them to make binary smaller. The description is extracted from the comment by generate-setting-docs.py.
The description should include the available options.

> cli: add support for Wake-on-LAN settings
g_set_error (error, 1, 0, "invalid option '%s'", err_token) in nmc_property_wired_set_wake_on_lan() should be translatable error message.

When setting wake-on-lan in nmcli editor, user doesn't know what options can be used. Print valid values at least in the error message. Later, we can make use of libnm metadata (when jk/libnm-nmcli-metadata branch is merged).

Comment 17 Jirka Klimes 2015-06-10 07:23:53 UTC
"wake-on-lan-password" as a secret should probably declare NM_SETTING_PARAM_SECRET flag. And use GET_SECRET() macro in nmcli to print it as <hidden>.

Comment 18 Lubomir Rintel 2015-06-10 09:26:10 UTC
(In reply to Jirka Klimes from comment #17)
> "wake-on-lan-password" as a secret should probably declare
> NM_SETTING_PARAM_SECRET flag. And use GET_SECRET() macro in nmcli to print
> it as <hidden>.

I don't think this is the case.

The "secret" here is used to extend the MAC address by a couple of octets in order to make it less likely to wake random machines by brute forcing the address. It's as really as secret as the MAC address -- it's transmitted unencrypted on the media and is of not much use once you know the MAC address.

Comment 19 Thomas Haller 2015-06-11 11:19:22 UTC
I know I said first "make it an external variable", but could we make it a function? Seems like more flexible for the future.

 NM_AVAILABLE_IN_1_2
+extern NMMetaFlag *nm_meta_flags_wake_on_lan;


Also, I would name it:
  nm_setting_wired_wake_on_lan_get_flags()
or
  nm_setting_wired_wake_on_lan_get_meta_flags()




char *nm_meta_flag_to_str (const NMMetaFlag *flags, int value)
extern NMMetaFlag *nm_meta_flags_wake_on_lan;

I would either both have "const NMMetaFlag *" or "NMMetaFlag *".
Maybe both const, because we can later remove the const-ness without braking API/ABI -- but not the other way around.

Comment 20 Beniamino Galvani 2015-06-12 07:08:56 UTC
(In reply to Lubomir Rintel from comment #18)
> (In reply to Jirka Klimes from comment #17)
> > "wake-on-lan-password" as a secret should probably declare
> > NM_SETTING_PARAM_SECRET flag. And use GET_SECRET() macro in nmcli to print
> > it as <hidden>.
> 
> I don't think this is the case.
> 
> The "secret" here is used to extend the MAC address by a couple of octets in
> order to make it less likely to wake random machines by brute forcing the
> address. It's as really as secret as the MAC address -- it's transmitted
> unencrypted on the media and is of not much use once you know the MAC
> address.

I agree, this shouldn't be treated as a secret.

Fixed with all the suggestions above, rebased on master and repushed, thanks.

Comment 21 Dan Williams 2015-06-18 20:01:57 UTC
> libnm-core: add Wake-on-LAN properties to NMSettingWired

Since the WOL password is basically a MAC address, the NMSetting documentation for the property should be updated to indicate that it looks like a valid MAC address.  Otherwise it seems like you could put anything there since it's a string.

> libnm-core: add NMMetaFlag utilities

For the Meta flags stuff, can't we just use the glib-mkenums functionality for this?  That auto-generates enum/flag <-> string functions for us based on templates that we create, see:

http://cgit.freedesktop.org/ModemManager/ModemManager/tree/build-aux/
http://cgit.freedesktop.org/ModemManager/ModemManager/tree/libmm-glib/generated/Makefile.am#n48

Doing it this way means doing the work once for the build system, and then every enum/flag gets the functions automatically and no more work is required from us.

> ifcfg-rh: add support for Wake-on-LAN ethtool options

Could we get some testcases for the parsing here?

Comment 22 Beniamino Galvani 2015-06-22 16:06:35 UTC
(In reply to Dan Williams from comment #21)
> > libnm-core: add Wake-on-LAN properties to NMSettingWired
>
> Since the WOL password is basically a MAC address, the NMSetting
> documentation for the property should be updated to indicate that it looks
> like a valid MAC address.  Otherwise it seems like you could put anything
> there since it's a string.

Updated.

> > libnm-core: add NMMetaFlag utilities
>
> For the Meta flags stuff, can't we just use the glib-mkenums functionality
> for this?  That auto-generates enum/flag <-> string functions for us based
> on templates that we create, see:

I modified the Makefiles to generate enum helpers according to template
files and now the following functions are generated for every enum:

 * ${enum_name}_get_string()              (only for regular enums)
 * ${enum_name}_build_string_from_mask()  (only for bitmask enums)
 * ${enum_name}_get_value()               (for both)

> > ifcfg-rh: add support for Wake-on-LAN ethtool options
>
> Could we get some testcases for the parsing here?

Branch repushed, thanks.

Comment 23 Thomas Haller 2015-06-26 12:55:51 UTC
>> build: add template files for glib-mkenums

this builds unconditionally accessors for a large number of enums, while we actually only expose/need a very small number of them.

Also, they become public API, while they weren't before (libnm-core/nm-core-enum-types.h), while their symbols don't get exposed. I think these should not become public API.


It builds them for libnm-glib, basically adding new API there...



Also, I dislike the names :)

 * ${enum_name}_get_string()              (only for regular enums)
 * ${enum_name}_build_string_from_mask()  (only for bitmask enums)
 * ${enum_name}_get_value()               (for both)

I would call them _to_string(), _from_string(), _create_string().

Comment 24 Thomas Haller 2015-06-26 12:59:02 UTC
For reasons stated before, I still prefer an API like:

  nm_meta_flag_to_string (nm_setting_wired_wake_on_lan_get_flags(), value);

with having many

  ${enum_name}_to_flags() functions,

over the alternative API.



(this is regardless of whether we generate it via mkenums or do it manually).

Comment 25 Thomas Haller 2015-06-26 13:02:32 UTC
(In reply to Thomas Haller from comment #24)
> For reasons stated before, I still prefer an API like:

the reason is, that a NMMetaFlags object can be used generically, i.e. if we add meta-data support to libnm, we could ask a setting to return a NMMetaFlags object and operate on it, instead of knowing which function to call.

Also, we can add new functions once:
  nm_meta_flag_build_from_other_string()
without having to add them for ~every~ enum type...

Comment 26 Beniamino Galvani 2015-07-02 08:21:55 UTC
(In reply to Thomas Haller from comment #23)
> >> build: add template files for glib-mkenums
>
> this builds unconditionally accessors for a large number of enums, while we
> actually only expose/need a very small number of them.
>
> Also, they become public API, while they weren't before
> (libnm-core/nm-core-enum-types.h), while their symbols don't get exposed. I
> think these should not become public API.

Good point.

(In reply to Thomas Haller from comment #24)
> For reasons stated before, I still prefer an API like:
>
>   nm_meta_flag_to_string (nm_setting_wired_wake_on_lan_get_flags(), value);

Right now glib-mkenum already generates for each enum a *_get_type()
function which returns a GType from which it's possible to do string
<-> value conversion. So, instead of defining a NMMetaFlags object
which would duplicate the enum conversion tables, we can simply use
that GType and define new functions like:

  char *nm_meta_flag_to_str (GType type, int value);
  gboolean nm_meta_flag_from_str (GType type, const char *str,
                                  int *out_value, char **err_token);

This has the advantage of reusing the existing glib-mkenum
infrastructure and adds only 2 new public functions.

Comment 27 Beniamino Galvani 2015-07-02 13:36:10 UTC
(In reply to Beniamino Galvani from comment #26)

> Right now glib-mkenum already generates for each enum a *_get_type()
> function which returns a GType from which it's possible to do string
> <-> value conversion. So, instead of defining a NMMetaFlags object
> which would duplicate the enum conversion tables, we can simply use
> that GType and define new functions like:
> 
>   char *nm_meta_flag_to_str (GType type, int value);
>   gboolean nm_meta_flag_from_str (GType type, const char *str,
>                                   int *out_value, char **err_token);
> 

Branch bg/wol-rh1141417-v2 implements this approach (I renamed functions
to have a nm_meta_enum_* prefix as it seems more appropriate -- they can
deal with both regular enums and 'flags' enums).

Comment 28 Dan Williams 2015-07-02 20:53:43 UTC
Is there a good reason to still have "meta" in the name?  These functions should work just as fine with any GType that's a flag/enum as with NM ones.  To me, having "meta" in the name doesn't mean anything really...

Comment 29 Beniamino Galvani 2015-07-06 13:09:59 UTC
(In reply to Dan Williams from comment #28)
> Is there a good reason to still have "meta" in the name?  These functions
> should work just as fine with any GType that's a flag/enum as with NM ones. 
> To me, having "meta" in the name doesn't mean anything really...

Right, I renamed the functions to nm_utils_enum_*. I also added support for default value from NM.conf and repushed to branch bg/wol-rh1141417-v2.

Comment 30 Thomas Haller 2015-07-09 15:13:39 UTC
(In reply to Beniamino Galvani from comment #29)
> (In reply to Dan Williams from comment #28)
> > Is there a good reason to still have "meta" in the name?  These functions
> > should work just as fine with any GType that's a flag/enum as with NM ones. 
> > To me, having "meta" in the name doesn't mean anything really...
> 
> Right, I renamed the functions to nm_utils_enum_*. I also added support for
> default value from NM.conf and repushed to branch bg/wol-rh1141417-v2.

I don't agree that "nm_utils_*" is a better name then "nm_meta_*". The "meta" does have a some meaning, in the sense of "meta-data". Information about data, how to convert an enum value to/fro string.

The name prefix would also be so that we don't stuff it all to nm-utils.c, but split some parts off to nm-meta.c.

Anyway... if you all prefer the former...



>> libnm-core: add enum conversion utilities

+    } else
+         g_assert_not_reached ();

just g_return_if_reached ().


  tores a ponter to a newl
          ^^^^^^

I think nm_utils_enum_from_str() should best effort parse the recognized flags and return them in @out_value.

I think it would be more useful if @err_token points inside @str where the invalid token starts. You could just clone @str, set all occurences of ',' to '\0', and walk though that string. If you find an invalid token, you know the offset inside your clone (and can calculate to return in @str).



+typedef enum {
+    NM_TEST_GENERAL_FLAGS_NONE = 0,
+    NM_TEST_GENERAL_FLAGS_FOO = (1 << 0),
+    NM_TEST_GENERAL_FLAGS_BAR = (1 << 1),
+    NM_TEST_GENERAL_FLAGS_BAZ = (1 << 2),
+} NMTestGeneralFlags;

why does it generate here a flags enum? Does it autodetect it?

could you also extend the test how it behaves if you have /*<flags>*/ and /*<skip>*/?



>> libnm-core: add Wake-on-LAN properties to NMSettingWired


+typedef enum {
/*< flags >*/ ?
+    NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT   = (1 << 0),



+#define NM_SETTING_WIRED_WAKE_ON_LAN_NONE    0
+#define NM_SETTING_WIRED_WAKE_ON_LAN_ALL     (((_NM_SETTING

why are they defines? Does <skip> not work?



>> core: configure Wake-on-LAN parameters for Ethernet devices

+         if (   nm_utils_enum_from_str (nm_setting_wired_wake_on_lan_get_type(),
+                                        value, (int *) &wol, NULL)
+             && wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)

is this the same way how keyfile parses the enum value? Maybe add an nm_keyfile_enum_from_string()?

Comment 31 Beniamino Galvani 2015-07-10 09:14:11 UTC
(In reply to Thomas Haller from comment #30)
> I don't agree that "nm_utils_*" is a better name then "nm_meta_*". The
> "meta" does have a some meaning, in the sense of "meta-data". Information
> about data, how to convert an enum value to/fro string.
> 
> The name prefix would also be so that we don't stuff it all to nm-utils.c,
> but split some parts off to nm-meta.c.
> 
> Anyway... if you all prefer the former...

I don't have a strong preference; maybe a slight one towards nm_utils...

> I think nm_utils_enum_from_str() should best effort parse the recognized
> flags and return them in @out_value.

Agree.

> I think it would be more useful if @err_token points inside @str where the
> invalid token starts. You could just clone @str, set all occurences of ','
> to '\0', and walk though that string. If you find an invalid token, you know
> the offset inside your clone (and can calculate to return in @str).

I think that the function would be typically used to parse a user-provided string and print the first invalid token on error. Which use case do you have in mind which requires @err_token to point inside @str?

> +typedef enum {
> +    NM_TEST_GENERAL_FLAGS_NONE = 0,
> +    NM_TEST_GENERAL_FLAGS_FOO = (1 << 0),
> +    NM_TEST_GENERAL_FLAGS_BAR = (1 << 1),
> +    NM_TEST_GENERAL_FLAGS_BAZ = (1 << 2),
> +} NMTestGeneralFlags;
> 
> why does it generate here a flags enum? Does it autodetect it?

Yes, because there are bit-shifts.

> could you also extend the test how it behaves if you have /*<flags>*/ and
> /*<skip>*/?

Ok.

> >> libnm-core: add Wake-on-LAN properties to NMSettingWired

> +#define NM_SETTING_WIRED_WAKE_ON_LAN_NONE    0
> +#define NM_SETTING_WIRED_WAKE_ON_LAN_ALL     (((_NM_SETTING
> 
> why are they defines? Does <skip> not work?

Yeah, good point.

> >> core: configure Wake-on-LAN parameters for Ethernet devices
> 
> +         if (   nm_utils_enum_from_str
> (nm_setting_wired_wake_on_lan_get_type(),
> +                                        value, (int *) &wol, NULL)
> +             && wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)
> 
> is this the same way how keyfile parses the enum value? Maybe add an
> nm_keyfile_enum_from_string()?

No, the property is a uint and is stored as a numeric value in keyfile; in the [connection] default settings is a string enum instead. Maybe it would be better to make it a uint also in [connection], what do you think?

Comment 32 Thomas Haller 2015-07-10 10:24:14 UTC
(In reply to Beniamino Galvani from comment #31)
> (In reply to Thomas Haller from comment #30)
> 
> > I think it would be more useful if @err_token points inside @str where the
> > invalid token starts. You could just clone @str, set all occurences of ','
> > to '\0', and walk though that string. If you find an invalid token, you know
> > the offset inside your clone (and can calculate to return in @str).
> 
> I think that the function would be typically used to parse a user-provided
> string and print the first invalid token on error. Which use case do you
> have in mind which requires @err_token to point inside @str?

Hm, ok, maybe not.


> > +typedef enum {
> > +    NM_TEST_GENERAL_FLAGS_NONE = 0,
> > +    NM_TEST_GENERAL_FLAGS_FOO = (1 << 0),
> > +    NM_TEST_GENERAL_FLAGS_BAR = (1 << 1),
> > +    NM_TEST_GENERAL_FLAGS_BAZ = (1 << 2),
> > +} NMTestGeneralFlags;
> > 
> > why does it generate here a flags enum? Does it autodetect it?
> 
> Yes, because there are bit-shifts.

I would not leave it to autodetection, but mark them explicitly.


> > >> core: configure Wake-on-LAN parameters for Ethernet devices
> > 
> > +         if (   nm_utils_enum_from_str
> > (nm_setting_wired_wake_on_lan_get_type(),
> > +                                        value, (int *) &wol, NULL)
> > +             && wol != NM_SETTING_WIRED_WAKE_ON_LAN_DEFAULT)
> > 
> > is this the same way how keyfile parses the enum value? Maybe add an
> > nm_keyfile_enum_from_string()?
> 
> No, the property is a uint and is stored as a numeric value in keyfile; in
> the [connection] default settings is a string enum instead. Maybe it would
> be better to make it a uint also in [connection], what do you think?

I think the syntax for the [connection] entries should be exactly the same as for keyfile.  See http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0c6a011e344612e607758626e51077b6edda66f5

Comment 33 Beniamino Galvani 2015-07-10 11:59:21 UTC
(In reply to Thomas Haller from comment #32)
> I would not leave it to autodetection, but mark them explicitly.

Ok.

> I think the syntax for the [connection] entries should be exactly the same
> as for keyfile.  See
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/
> ?id=0c6a011e344612e607758626e51077b6edda66f5

I will change the connection entry to be in numeric format then.

Comment 34 Dan Williams 2015-07-12 01:28:22 UTC
I see thaller's argument on naming, but I still have a slight preference for nm_utils.

Comment 35 Beniamino Galvani 2015-07-15 16:09:38 UTC
(In reply to Beniamino Galvani from comment #33)
> (In reply to Thomas Haller from comment #32)
> > I think the syntax for the [connection] entries should be exactly the same
> > as for keyfile.  See
> > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/
> > ?id=0c6a011e344612e607758626e51077b6edda66f5
> 
> I will change the connection entry to be in numeric format then.

Pushed a fixup to branch bg/wol-rh1141417-v2. The downside is that the user now must manually compute the bitmask for the value in NetworkManager.conf, for example:

 [connection]
 802-3-ethernet.wake-on-lan=12

to set unicast,multicast mode. Probably we should also document the allowed values in the manual page?

Comment 36 Thomas Haller 2015-07-16 16:45:30 UTC
(In reply to Beniamino Galvani from comment #35)
> (In reply to Beniamino Galvani from comment #33)
> > (In reply to Thomas Haller from comment #32)
> > > I think the syntax for the [connection] entries should be exactly the same
> > > as for keyfile.  See
> > > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/
> > > ?id=0c6a011e344612e607758626e51077b6edda66f5
> > 
> > I will change the connection entry to be in numeric format then.
> 
> Pushed a fixup to branch bg/wol-rh1141417-v2. The downside is that the user
> now must manually compute the bitmask for the value in NetworkManager.conf,
> for example:
> 
>  [connection]
>  802-3-ethernet.wake-on-lan=12
> 
> to set unicast,multicast mode. Probably we should also document the allowed
> values in the manual page?

IMO that is acceptable for now. Later, we should do what http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0c6a011e344612e607758626e51077b6edda66f5 suggests, then we can have more elaborate parsing.

But I think it's more important, to have a consistent behavior. (well, it's already broken, but ~as good as currently possible~).



NMSettingWiredWakeOnLan lacks /*<flags>*/




Otherwise LGTM

Comment 37 Dan Williams 2015-07-17 23:31:43 UTC
LGTM, though for the config defaults I think we should get away from "802-3-ethernet" and just use "ethernet".  What do you guys think?

Comment 38 Beniamino Galvani 2015-07-20 12:37:58 UTC
(In reply to Dan Williams from comment #37)
> LGTM, though for the config defaults I think we should get away from
> "802-3-ethernet" and just use "ethernet".  What do you guys think?

Maybe "802-3-ethernet.wake-on-lan" has the advantage that it follows the "setting.property" naming  so if one day we'll want to deal with default values in a generic way, it will be easier. But it's just a name and we can easily translate between the two, so "ethernet" seems a more appropriate choice for a user-editable configuration file.

Pushed some fixups to bg/wol-rh1141417-v2.

Comment 39 Beniamino Galvani 2015-07-24 12:23:14 UTC
Renamed the default config property to "ethernet.wake-on-lan" and merged to master:
  0398ee0 merge: ethernet: add Wake-on-LAN support

Comment 45 Vladimir Benes 2015-09-07 10:22:48 UTC
I tried to set magic (and that worked) but I wanted to set disabled, too. This is not easily possible as default seems to be g at least on my card (not sure how wide spreaded this  behaviour is).

I searched for disabled but all I got was:

nmcli> set 802-3-ethernet.wake-on-lan disabled
Error: failed to set 'wake-on-lan' property: invalid option 'disabled', use  a combination of phy,unicast,multicast,broadcast,arp,magic or 'default'

As Beniamino told me this can be done by:
1. nmcli c modify "Auto Ethernet" 802-3-ethernet.wake-on-lan " "

2. nmcli> set 802-3-ethernet.wake-on-lan
Enter 'wake-on-lan' value:  
^^ you have to add space as nothing means default

3. set default in nmcli and add option wol d into ifcfg file.

Options 1 and 2 seem to be pretty unintuitive and option 3 can be easily overwritten from nmcli. The bug seems to be fixed otherwise, I've filed it as bug 1260584

Comment 46 Vladimir Benes 2015-09-07 15:29:31 UTC
I've tested and verified that w-o-l functionality is correctly added on 5 scenarios:

add magic to ethernet.wake-on-lan to enable (g)
add " " to ethernet.wake-on-lan to disable (d)
add default to ethernet.wake-on-lan to get what default (g or d)

read from ifcfg file when default is set
override ifcfg option via nmcli one

Comment 47 Eric Work 2015-09-24 03:55:01 UTC
Just wanted to comment that since upgrading to NetworkManager 1.0.6 on Fedora 22 WOL stopped working until I set this new setting (802-3-ethernet.wake-on-lan) to "magic".  When it stopped working I first tried reseting and changing some BIOS settings then a search for "wakeonlan" on bugzilla found this bug.  With some tips from the Arch Linux wiki [1] it's working now.  It seems "default" is not the same as what it was before this setting was introduced.

[1] https://wiki.archlinux.org/index.php/Wake-on-LAN#NetworkManager

Comment 48 Beniamino Galvani 2015-09-24 16:06:05 UTC
(In reply to Eric Work from comment #47)
> It seems "default" is not the same as what it was before
> this setting was introduced.

Yes, there is already a bug open for this issue on the upstream bugzilla:

https://bugzilla.gnome.org/show_bug.cgi?id=755182

Comment 49 errata-xmlrpc 2015-11-19 10:57:27 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2015-2315.html

Comment 50 Thomas Haller 2016-02-25 18:12:10 UTC
Created attachment 1130611 [details]
[patch] obsolete work in prograss by danw [danw/wip/ethtool]

Dan had an old work-in-progress to add wake-on-lan support in branch danw/wip/ethtool. As the patch is obsolete, I attach it here for history.

The patch applies on commit aa1b262091096357dfd35d2558de8a90048fd53c.


Note You need to log in before you can comment on or make changes to this bug.