doesn't compile:

../libnm-core/nm-setting-ip6-config.c:191:59: error: incompatible pointer types passing 'NMSettingIPConfig *' (aka 'struct _NMSettingIPConfig *') to parameter of type 'NMSettingIP6Config *' (aka 'struct _NMSettingIP6Config *') [-Werror,-Wincompatible-pointer-types]
        addr_gen_mode = nm_setting_ip6_config_get_addr_gen_mode (s_ip);


while at it, could you rebase it on master? Thanks

Rebased.

>> setting-ip6-config: add addr-gen-mode property

I know we talked about it (and I forgot what my opinion was back then), but why is this property a string and not an enum?

Also, if you do strong verification in nm-setting-ip6-config:verify(), then we have a problem with extending this property in the future.

I would have the gobject-property of type gint, but all other values using an enum, without strong validation in verify() to accept unknown value.
(of course, all code must somehow treat unknown values properly).

(In reply to Thomas Haller from comment #3)
> >> setting-ip6-config: add addr-gen-mode property
> 
> I know we talked about it (and I forgot what my opinion was back then), but
> why is this property a string and not an enum?

My memory is not too clear too. I believe the arguments were: a string looks more sensible in the D-Bus API and it's more easily interpreted without the need to translate it in client.

> Also, if you do strong verification in nm-setting-ip6-config:verify(), then
> we have a problem with extending this property in the future.

Hm, yeah; we should probably only validate it on server?

> I would have the gobject-property of type gint, but all other values using
> an enum, without strong validation in verify() to accept unknown value.
> (of course, all code must somehow treat unknown values properly).

I think this speaks in favor of a string too? Just reject unknown values on DBus and fail to verify an existing connection with unknown value when loading it (should not really happen unless someone downgrades and failing to verify the connection is probably the most sane thing to do in such case?).

Rebased, split up into logical parts and re-pushed.

Pushed fixup with machine-secret(5) support.

>> setting-ip6-config: add addr-gen-mode property

I still prefer enums over string type for NMSettingIP6Config:addr-gen-mode.
Note that virtually the only enum-like property which is stored as string is ipvx.method.
And it seems enum types work well for us.

Let's at least hear what others think about it.




typo:
the host part of the address to stay constant making, making it possible
                                             ^^^^^^^


>> ifcfg-rh: add support for addr-gen-mode property

gs_free char *addr_gen_mode = NULL;
?


Could you move this commit before the nmcli one?



>> rdisc: move address generation into NMRDisc from NMLNDPRdisc
    
+    if (!complete_address (rdisc, new)) {
+         _LOGW ("Can't generate an IPv6 address");

have complete_address() log the failure, with possibly giving more details about what failed. And give all the messages that are logged by the function a prefix "complete-address:".



To bad we cannot conviniently log NMRDiscAddress.
How about adding a function

  NMPlatformIP6Address *nm_rdisc_address_to_platform (
        const NMRDiscAddress *src,
        NMPlatformIP6Address *dst) {

        memset (dst, 0, sizeof (*dst));
        dst->address = src->address;
        ...
        return dst;

  }

and then:

  nm_platfrom_ip6_address_to_string (nm_rdisc_address_to_platform (new, &buf));

Note that this function is very much useful in rdisc_config_changed() too where we already do this conversion.



>> core: support IPv6 duplicate address detection

+              if (   IN6_IS_ADDR_LINKLOCAL (&addr->address)
+                     && !(addr->flags & IFA_F_DADFAILED)) {

whitespace


g_signal_emit_by_name (rdisc, NM_RDISC_CONFIG_CHANGED, NM_RDISC_CONFIG_ADDRESSES);

expects as argument a config-map, not a string. I think it's quite ugly to have such a signal, and I would rather refactor the whole signal to have a property NM_RDISC_CONFIG and then just g_object_notify().



  /* LL generation already running and we can't do anything about it */
  _LOGW (LOGD_IP6, "DAD failed for an EUI-64 address");

I don't understand why this "if(priv->linklocal6_timeout_id)" is there, and neither the comment nor the log-line helps.



The action in device_ipx_changed() should be delayed. You are there in the middle of a callback from platform (which possibly was triggered by a call from device to platform). Possibly remember what you intend to do, g_idle_add(), check if the conditions are still the same, and do it.



>> core: add support for RFC7217 stable privacy addressing


+         if (!g_strcmp0 (rdisc->addr_gen_mode,
+                            NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY))

whitespace



+ _LOGD ("Adding an stable-privacy address");
yes, but which address? (see above)


Could we have common prefixes to logging lines that are related? I know, we log already __FUNC__, but I actually dislike that because it causes our log-files to be unaligned and with lot's of noise. If you configure logging.backend=journal you don't get those (and I hope one day to be that the default).





Can we have nm_utils_ipv6_addr_set_stable_privacy() to return a GError instead of logging itself? nm_utils_ipv6_addr_set_stable_privacy() doesn't know the context about why to log and you end up with two logging lines:


check_and_add_ipv6ll_addr()
  Using IPv6 stable-privacy addressing
  can not generate a secret for IPv6 stable privacy address
  failed to generate and adderss; IPv6 cannot continue

urgh.

instead of message telling what failed why:

  if (!nm_utils_ipv6_addr_set_stable_privacy(...)) {
      LOG(failed to generate IPv6 stable privay address: failed to create 
          secret key)
      return;
  }
  LOG(Using IPv6 stable-privacy addressing)




Same in complete_address().



typo: adderss





Could you please rebase the branch (and order the fixup commits)

(In reply to Thomas Haller from comment #7)
> >> setting-ip6-config: add addr-gen-mode property
>
> I still prefer enums over string type for NMSettingIP6Config:addr-gen-mode.
> Note that virtually the only enum-like property which is stored as string is
> ipvx.method.
> And it seems enum types work well for us.
>
> Let's at least hear what others think about it.

Fair enough. Adjusted to an enum.

> typo:
> the host part of the address to stay constant making, making it possible

Fixed.

> >> ifcfg-rh: add support for addr-gen-mode property
>
> gs_free char *addr_gen_mode = NULL;
> ?

Gone with the switch to enum.

> Could you move this commit before the nmcli one?

Done.

> >> rdisc: move address generation into NMRDisc from NMLNDPRdisc
>
> +    if (!complete_address (rdisc, new)) {
> +         _LOGW ("Can't generate an IPv6 address");
>
> have complete_address() log the failure, with possibly giving more details
> about what failed.


Just removed that. For the stable-privacy addresses the nm_utils_ipv6_addr_set_stable_privacy() already complains if it's unable to proceed. For the EUI64 failure case the logging is already there.

> And give all the messages that are logged by the function
> a prefix "complete-address:".

Done.

> To bad we cannot conviniently log NMRDiscAddress.
> How about adding a function
>
>   NMPlatformIP6Address *nm_rdisc_address_to_platform (
>         const NMRDiscAddress *src,
>         NMPlatformIP6Address *dst) {
>
>         memset (dst, 0, sizeof (*dst));
>         dst->address = src->address;
>         ...
>         return dst;
>
>   }
>
> and then:
>
>   nm_platfrom_ip6_address_to_string (nm_rdisc_address_to_platform (new,
> &buf));
>
> Note that this function is very much useful in rdisc_config_changed() too
> where we already do this conversion.

Where would that be useful? Logging and address that we managed not to generate? :)

> >> core: support IPv6 duplicate address detection
>
> +              if (   IN6_IS_ADDR_LINKLOCAL (&addr->address)
> +                     && !(addr->flags & IFA_F_DADFAILED)) {
>
> whitespace

Fixed.

> g_signal_emit_by_name (rdisc, NM_RDISC_CONFIG_CHANGED,
> NM_RDISC_CONFIG_ADDRESSES);
>
> expects as argument a config-map, not a string. I think it's quite ugly to
> have such a signal, and I would rather refactor the whole signal to have a
> property NM_RDISC_CONFIG and then just g_object_notify().


Hmm, this somehow falls out of scope of this branch though.
Maybe open a separate ticket?

>   /* LL generation already running and we can't do anything about it */
>   _LOGW (LOGD_IP6, "DAD failed for an EUI-64 address");
>
> I don't understand why this "if(priv->linklocal6_timeout_id)" is there, and
> neither the comment nor the log-line helps.

Tried to add a better expplanation.

> The action in device_ipx_changed() should be delayed. You are there in the
> middle of a callback from platform (which possibly was triggered by a call
> from device to platform). Possibly remember what you intend to do,
> g_idle_add(), check if the conditions are still the same, and do it.

Moved the DAD handling into queued_ip6_config_change(). We're enqueueing it
for address changes anyway and we do request LL addresses there already;
I guess that's the correct way to deal with the other addresses too.

> >> core: add support for RFC7217 stable privacy addressing
>
>
> +         if (!g_strcmp0 (rdisc->addr_gen_mode,
> +
> NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY))
>
> whitespace

Gotten rid of the strcmp altogether when switching to enum.

>
>
>
> + _LOGD ("Adding an stable-privacy address");
> yes, but which address? (see above)

Well, we don't know at this point. I don't think it matters too much, this is a
debug-level statement anyway.

> Could we have common prefixes to logging lines that are related? I know, we
> log already __FUNC__, but I actually dislike that because it causes our
> log-files to be unaligned and with lot's of noise. If you configure
> logging.backend=journal you don't get those (and I hope one day to be that
> the default).
>
>
>
>
>
> Can we have nm_utils_ipv6_addr_set_stable_privacy() to return a GError
> instead of logging itself? nm_utils_ipv6_addr_set_stable_privacy() doesn't
> know the context about why to log and you end up with two logging lines:

Yes, good idea. Done.

> check_and_add_ipv6ll_addr()
>   Using IPv6 stable-privacy addressing
>   can not generate a secret for IPv6 stable privacy address
>   failed to generate and adderss; IPv6 cannot continue
>
> urgh.
>
> instead of message telling what failed why:
>
>   if (!nm_utils_ipv6_addr_set_stable_privacy(...)) {
>       LOG(failed to generate IPv6 stable privay address: failed to create
>           secret key)
>       return;
>   }
>   LOG(Using IPv6 stable-privacy addressing)
>
>
>
>
> Same in complete_address().

Done.

> typo: adderss

Done.

> Could you please rebase the branch (and order the fixup commits)

Done.

> cli: add addr-gen-mode property

	nm_setting_ip6_config_addr_gen_mode_get_type;
+       nm_setting_ip6_config_get_addr_gen_mode;

This should go in the first commit.

Pushed a couple of cosmetic fixups. The rest LGTM.

(In reply to Beniamino Galvani from comment #9)
> > cli: add addr-gen-mode property
> 
> 	nm_setting_ip6_config_addr_gen_mode_get_type;
> +       nm_setting_ip6_config_get_addr_gen_mode;
> 
> This should go in the first commit.
> 
> Pushed a couple of cosmetic fixups. The rest LGTM.

Thank you. Branch updated.

>> setting-ip6-config: add addr-gen-mode property

Pushed a fixup, I think the GObject property must be of integer type.


>> keyfile: add support for addr-gen-mode property

This commit needs adjustment if the property is no longer of GEnum type (due to previous fixup).

keyfile usually writes/reads enums as integers. Why do you want to change that for addr-gen-mode?





+         _LOGI (LOGD_IP6, "ipv6: duplicate address check failed for the %s address",
+                    nm_platform_ip6_address_to_string (addr));

whitespace.




>> core: support IPv6 duplicate address detection


nm_rdisc_dad_failed() always emits a signal, even if there was nothing to do.





+         _LOGD ("rdisc: DAD failed for a discovered address\n");
                                                           ^^^^^

and for *which* address? you said:

> Where would that be useful? Logging and address that we managed not to
> generate? :)

it would be useful to be able to follow a prefix and see which addresses are generated (for a certain prefix). And when we generate new addresses (and if we fail to do so, why). Why can the logging statements in complete_address() not mention the prefix/address that they talk about.


+             || (change_type == NM_PLATFORM_SIGNAL_REMOVED && addr->flags & IFA_F_TENTATIVE)) {

Why did the address got removed? I think if something externally removes an address, we should not regenerate a different prefix. IMO you should remove the address entirely (until the next RA).



>> core: add support for RFC7217 stable privacy addressing
    
+              g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+                             "Key is too short to be usable");

+                                                     nm_device_get_iface (self),
+                                                  nm_connection_get_uuid (connection),
+                                                     priv->linklocal6_dad_counter++,


whitespace




+    g_checksum_update (sum, (const guchar *) &dad_counter, sizeof (dad_counter));

dad-counter is a guint, thus both size and endianess is platform dependent (ok, size is probably everywhere 32 bit, anyway).
To generate the same hash on every platform, either convert to string, or better:

  guint32 tmp;

  tmp = htonl (dad_counter);


+    g_checksum_update (sum, (const guchar *) ifname, -1);
+    if (uuid)
+         g_checksum_update (sum, (const guchar *) uuid, -1);


I would also hash the trailing '\0'. Like:

+    g_checksum_update (sum, (const guchar *) ifname, strlen (ifname) + 1);

that way, the following all hash differently:
  ifname="a", uuid="b"  => "a\0b\0"
  ifname="ab", uuid=""  => "ab\0\0"
  ifname="", uuid="ab"  => "\0ab\0"



For similar reason I would explicitly mark whether a UUID is present.

    if (uuid)
         g_checksum_update (sum, (const guchar *) uuid, strlen (uuid) + 1);


And also hash the keylenght itself and the whether uuid is present or not.
Basically, you serialize those values together (before hashing). I think you should also be able to deserialize them unambiguously.




I think nm_utils_ipv6_addr_set_stable_privacy() should be made testable. And then add test cases with hard-coded result. Note that after release, we must never change the algorithm.

>>> setting-ip6-config: add addr-gen-mode property
>
>Pushed a fixup, I think the GObject property must be of integer type.

Thanks.

>>> keyfile: add support for addr-gen-mode property
>
>This commit needs adjustment if the property is no longer of GEnum type (due to previous fixup).

Done.

>keyfile usually writes/reads enums as integers. Why do you want to change that for addr-gen-mode?

Yeah, and it's dead ugly. We should probably convert the rest to strings too (retaining the ability to load integers, of course).

>+         _LOGI (LOGD_IP6, "ipv6: duplicate address check failed for the %s address",
>+                    nm_platform_ip6_address_to_string (addr));
>
>whitespace.

Fixed.

>>> core: support IPv6 duplicate address detection
>
>
>nm_rdisc_dad_failed() always emits a signal, even if there was nothing to do.

Changed.

>+         _LOGD ("rdisc: DAD failed for a discovered address\n");
>                                                           ^^^^^
>
>and for *which* address? you said:
>
>> Where would that be useful? Logging and address that we managed not to
>> generate? :)
>
>it would be useful to be able to follow a prefix and see which addresses are generated (for a certain prefix). And when we generate new addresses (and if we fail to do so, why). Why can the logging statements in complete_address() not mention the prefix/address that they talk about.

I'm not too sure. This is a debugging statement and I'm not convinced that it's worth adding any extra complexity for this. The actual address is logged on info level from queued_ip6_config_change(), user knows which address is the bad one. If it disappears and the user believes it should have been replaced then the debug level statement tells him why.

>+             || (change_type == NM_PLATFORM_SIGNAL_REMOVED && addr->flags & IFA_F_TENTATIVE)) {
>
>Why did the address got removed? I think if something externally removes an address, we should not regenerate a different prefix. IMO you should remove the address entirely (until the next RA).

Can't tell. However it was tentative, which means odds are are it got removed due to a DAD failure.

Sure, the user can add an address and quickly remove it while it's tentative. He probably shouldn't do that.

We should eventually fix kernel to indicate IFA_F_DADFAILED upon such events, but at the moment we can't do any better.

>>> core: add support for RFC7217 stable privacy addressing
>
>+              g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
>+                             "Key is too short to be usable");
>
>+                                                     nm_device_get_iface (self),
>+                                                  nm_connection_get_uuid (connection),
>+                                                     priv->linklocal6_dad_counter++,
>
>
>whitespace

Fixed.

>+    g_checksum_update (sum, (const guchar *) &dad_counter, sizeof (dad_counter));
>
>dad-counter is a guint, thus both size and endianess is platform dependent (ok, size is probably everywhere 32 bit, anyway).
>To generate the same hash on every platform, either convert to string, or better:
>
>  guint32 tmp;
>
>  tmp = htonl (dad_counter);

Done.

>
>
>+    g_checksum_update (sum, (const guchar *) ifname, -1);
>+    if (uuid)
>+         g_checksum_update (sum, (const guchar *) uuid, -1);
>
>
>I would also hash the trailing '\0'. Like:
>
>+    g_checksum_update (sum, (const guchar *) ifname, strlen (ifname) + 1);
>
>that way, the following all hash differently:
>  ifname="a", uuid="b"  => "a\0b\0"
>  ifname="ab", uuid=""  => "ab\0\0"
>  ifname="", uuid="ab"  => "\0ab\0"
>
>
>
>For similar reason I would explicitly mark whether a UUID is present.
>
>    if (uuid)
>         g_checksum_update (sum, (const guchar *) uuid, strlen (uuid) + 1);
>
>
>And also hash the keylenght itself and the whether uuid is present or not.
>Basically, you serialize those values together (before hashing). I think you should also be able to deserialize them unambiguously.

Done.

>I think nm_utils_ipv6_addr_set_stable_privacy() should be made testable. And then add test cases with hard-coded result. Note that after release, we must never change the algorithm.

Added a test.

Pushed a new version.

pushed fixup commits. Feel free to reject.

The rest LGTM.

(In reply to Thomas Haller from comment #13)
> pushed fixup commits. Feel free to reject.

Thank you, applied.

> The rest LGTM.

master:

9b31d6e merge: branch 'lr/stable-privacy-rfc7217'
e9dfdfe libnm-core: default to ip6.addr-gen-mode=stable-privacy
e603c86 core: add support for RFC7217 stable privacy addressing
f85728e core: support IPv6 duplicate address detection
b3e0811 rdisc: move address generation into NMRDisc from NMLNDPRdisc
4d6649f cli: add addr-gen-mode property
60d2504 ifcfg-rh: add support for addr-gen-mode property
f70c8f3 keyfile: add support for addr-gen-mode property
60811b4 setting-ip6-config: add addr-gen-mode property