Bug 1141417

(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?

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.

*** Bug 1203102 has been marked as a duplicate of this bug. ***

Branch bg/wol-rh1141417 implements Wake-on-LAN settings, if we agree that NM should handle them.

>> 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

(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.

(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.

(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.

(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.

(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().

(In reply to Thomas Haller from comment #13)

Fixed and repushed.

All commits LGTM

> 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).

"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>.

(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 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.

(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.

> 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?

(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.

>> 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().

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).

(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...

(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.

(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).

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...

(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.

(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()?

(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?

(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

(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.

I see thaller's argument on naming, but I still have a slight preference for nm_utils.

(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?

(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

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?

(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.

Renamed the default config property to "ethernet.wake-on-lan" and merged to master:
  0398ee0 merge: ethernet: add Wake-on-LAN support

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

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

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

(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

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

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.