GNOME Bugzilla – Bug 765851
[review] lr/connection-token: support configuring IPv6 tokenized identifier tokens
Last modified: 2016-06-07 15:48:25 UTC
0d9266f ifcfg-rh: drop IPV6_ADDR_GEN_MODE=stable-privacy when the mode is eui64 9444401 core-utils: add conversions of ipv6 tokens 4cd4feb linux: use the utility functions to convert the token from platform 850e522 setting-ip6-config: add token property fcae7a2 platform: add capability to set the tokenized interface identifier 3fd4fd6 device: use the token set in platform when generating a connection 1b439f6 device: set the iid to rdisc from connection's property a60fd16 device: set token in platform when applying the ip6 configuration https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/connection-token
>> core-utils: add conversions of ipv6 tokens + || in6addr->s6_addr[4] + || in6addr->s6_addr[5] + || in6addr->s6_addr[6] + || in6addr->s6_addr[7]) + return FALSE; + + if ( in6addr->s6_addr[8] + || in6addr->s6_addr[9] + || in6addr->s6_addr[10] indention. nm_utils_inet6_is_token() should not become public API. Or otherwise, it would need Since: and symbol-version-entry. Move to "nm-core-internal.h" instead. nm_utils_ip6_to_iid (struct in6_addr *i6_token, NMUtilsIPv6IfaceId *iid) ; nm_utils_iid_to_ip6 (NMUtilsIPv6IfaceId *iid, struct in6_addr *i6_token); nm_utils_token_to_iid (const char *token, NMUtilsIPv6IfaceId *iid) ; nm_utils_iid_to_token (NMUtilsIPv6IfaceId *iid); I think, stuff that is related should have a common prefix. In this case, its all about NMUtilsIPv6IfaceId. So, something like nm_utils_inet6_iid_from_addr nm_utils_inet6_iid_to_addr nm_utils_inet6_iid_from_token Optimally, they move closer to _NMUtilsIPv6IfaceId, and adjust a common naming scheme also to the existing: nm_utils_get_ipv6_interface_identifier nm_utils_ipv6_addr_set_interface_identfier nm_utils_ipv6_interface_identfier_get_from_addr (I mean renaming existing functions :) , so that this is somehow consistent) It seems to me, that nm_utils_ip6_to_iid() should just proceed with whatever is there. If some other component wants to enforce some rules, it should call nm_utils_inet6_is_token() itself. Preferably, let it return iid, then if (nm_utils_ip6_to_iid (i6_token, &iid)) *out_iid = iid; becomes if (nm_utils_inet6_is_token (i6_token)) *out_iid = nm_utils_ip6_to_iid (i6_token, &iid); It's not clear, what to do when kernel sends a bogus i6_token. Assume it's all-zero? Fine with me, but nm_utils_ip6_to_iid() should still set iid. >> linux: use the utility functions to convert the token from platform please review als th/platform-inet6-token >> setting-ip6-config: add token property verify() *must* return either NORMALIZABLE or NORMALIZEABLE_ERROR if the token needs normization. OTOH, _normalize_ip_config() must not touch the setting if it is not normalizable (inet_pton fails).
(In reply to Thomas Haller from comment #1) > >> core-utils: add conversions of ipv6 tokens > > > + || in6addr->s6_addr[4] > + || in6addr->s6_addr[5] > + || in6addr->s6_addr[6] > + || in6addr->s6_addr[7]) > + return FALSE; > + > + if ( in6addr->s6_addr[8] > + || in6addr->s6_addr[9] > + || in6addr->s6_addr[10] > > > indention. Fixed. > nm_utils_inet6_is_token() should not become public API. Or otherwise, it > would need Since: and symbol-version-entry. > > Move to "nm-core-internal.h" instead. Done > nm_utils_ip6_to_iid (struct in6_addr *i6_token, NMUtilsIPv6IfaceId *iid) ; > nm_utils_iid_to_ip6 (NMUtilsIPv6IfaceId *iid, struct in6_addr *i6_token); > nm_utils_token_to_iid (const char *token, NMUtilsIPv6IfaceId *iid) ; > nm_utils_iid_to_token (NMUtilsIPv6IfaceId *iid); > > I think, stuff that is related should have a common prefix. In this case, > its all about NMUtilsIPv6IfaceId. So, something like > nm_utils_inet6_iid_from_addr > nm_utils_inet6_iid_to_addr > nm_utils_inet6_iid_from_token > Optimally, they move closer to _NMUtilsIPv6IfaceId, and adjust a common > naming scheme also to the existing: > > nm_utils_get_ipv6_interface_identifier > nm_utils_ipv6_addr_set_interface_identfier > nm_utils_ipv6_interface_identfier_get_from_addr > > (I mean renaming existing functions :) , so that this is somehow consistent) Turns out there's already nm_utils_ipv6_addr_set_interface_identfier() and nm_utils_ipv6_interface_identfier_get_from_addr(). I'm using them now and adjusting the rest for consistency. > It seems to me, that nm_utils_ip6_to_iid() should just proceed with whatever > is there. If some other component wants to enforce some rules, it should > call nm_utils_inet6_is_token() itself. Preferably, let it return iid, then > > if (nm_utils_ip6_to_iid (i6_token, &iid)) > *out_iid = iid; > > becomes > > if (nm_utils_inet6_is_token (i6_token)) > *out_iid = nm_utils_ip6_to_iid (i6_token, &iid); > > It's not clear, what to do when kernel sends a bogus i6_token. Assume it's > all-zero? Fine with me, but nm_utils_ip6_to_iid() should still set iid. Undefined, I'd say. Nevertheless, it currently just uses the host part which sounds reasonable to me. > >> linux: use the utility functions to convert the token from platform > > please review als th/platform-inet6-token Folded the branch in. > >> setting-ip6-config: add token property > > verify() *must* return either NORMALIZABLE or NORMALIZEABLE_ERROR if the > token needs normization. > OTOH, _normalize_ip_config() must not touch the setting if it is not > normalizable (inet_pton fails). Fixed. https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/connection-token
>> core-utils: add conversions of ipv6 tokens I think nm_utils_inet6_interface_identfier_to_token() should accept a string buffer (like nm_utils_inet6_ntop(), nm_platform_link_to_string(), etc). We can always allow NULL for fallback to a static buffer, but having no buffer argument limits the usability of the function. E.g. if nm_platform_link_to_string() would be fixed to use nm_utils_inet6_interface_identfier_to_token() -- which it should(!), the following would fail for unexpected reasons: _LOGD ("set token of link %s to %s", nm_platform_link_to_string (pllink, buffer, sizeof (buffer)), nm_utils_inet6_interface_identfier_to_token (&token)); pllink->inet6_token = token; + case PROP_TOKEN: + g_free (priv->token); + case PROP_TOKEN: + g_value_set_string (value, priv->token); + break; whitespace. IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode. Documentation doesn't match behavior of verify(), also allows tokens with stable-privacy mode. + tmp = svGetValue (ifcfg, "IPV6_TOKEN", FALSE); + if (tmp) { + g_object_set (s_ip6, + NM_SETTING_IP6_CONFIG_TOKEN, tmp, + NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE, NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_EUI64, + NULL); reading one property from ifcfg-rh, should only set the TOKEN. this wrongly handles: IPV6_ADDR_GEN_MODE=stable TOKEN=::53 + value = nm_setting_ip6_config_get_token (NM_SETTING_IP6_CONFIG (s_ip6)); + if (value) + svSetValue (ifcfg, "IPV6_TOKEN", value, FALSE); should be: + value = nm_setting_ip6_config_get_token (NM_SETTING_IP6_CONFIG (s_ip6)); + svSetValue (ifcfg, "IPV6_TOKEN", value, FALSE); >> platform: add capability to set the tokenized interface identifier We don't need the token set in platform for out address mode generation, ^^^ maybe _nl_msg_new_link_set_afspec() should not even nest_start(IFLA_AF_SPEC) if there are no properties. Add if (addr_gen_mode < 0 && !iid) return TRUE; at the beginning. + _LOGD ("link: change %d: token: set IPv6 address generation token to %s", + ifindex, nm_utils_hwaddr_ntoa (&iid, sizeof (iid))); nm_utils_inet6_interface_identfier_to_token()? nm_platform_link_set_ipv6_token (NMPlatform *self, int ifindex, NMUtilsIPv6IfaceId iid); didn't you say, that the token cannot be set to "::"? Then, nm_platform_link_set_ipv6_token() should have g_return_val_if_fail (iid.id, FALSE); and optionally, _nl_msg_new_link_set_afspec(), if (iid) { nm_assert (iid->id); ... >> device: set the iid to rdisc from connection's property - NMUtilsIPv6IfaceId iid; + const NMPlatformLink *pllink; belongs to previous commit. nm_device_get_ip_iface_identifier() should only accept the token the addr-gen-mode is EUI64. ip6_config_merge_and_apply() sets now the token every time. That doesn't seem right. - if (!nm_device_get_ip_iface_identifier (self, &iid)) { + /* Don't use nm_device_get_ip_iface_identifier() -- we don't want to take + * the token into account for the link-local address . No particular good + * reason for that other than that the kernel doesn't do that either. */ + if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid)) { Could we instead add an argument "consider_setting" to nm_device_get_ip_iface_identifier(). I find it confusing that there exist direct calls to the virtual method which bypass the wrapper. if addrconf6_start_with_link_ready() fails to get the iid, I think it still should call "nm_rdisc_set_iid(priv->rdisc, NULL);" to clear it. pushed a few commits.
(In reply to Thomas Haller from comment #3) > >> core-utils: add conversions of ipv6 tokens > > I think nm_utils_inet6_interface_identfier_to_token() should accept a string > buffer (like nm_utils_inet6_ntop(), nm_platform_link_to_string(), etc). > > We can always allow NULL for fallback to a static buffer, but having no > buffer argument limits the usability of the function. Ok, done. > E.g. if nm_platform_link_to_string() would be fixed to use > nm_utils_inet6_interface_identfier_to_token() -- which it should(!), the > following would fail for unexpected reasons: > > _LOGD ("set token of link %s to %s", > nm_platform_link_to_string (pllink, buffer, sizeof (buffer)), > nm_utils_inet6_interface_identfier_to_token (&token)); > pllink->inet6_token = token; Done too. > + case PROP_TOKEN: > + g_free (priv->token); > + case PROP_TOKEN: > + g_value_set_string (value, priv->token); > + break; > > whitespace. Fixed. > IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode. > > Documentation doesn't match behavior of verify(), also allows tokens with > stable-privacy mode. I think it's correct? It only makes a statement on usefulness; you can set the tokenized iid even when you don't use it to construct an address; but that's not too useful at this point. > + tmp = svGetValue (ifcfg, "IPV6_TOKEN", FALSE); > + if (tmp) { > + g_object_set (s_ip6, > + NM_SETTING_IP6_CONFIG_TOKEN, tmp, > + NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE, > NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_EUI64, > + NULL); > > reading one property from ifcfg-rh, should only set the TOKEN. > > this wrongly handles: > > IPV6_ADDR_GEN_MODE=stable > TOKEN=::53 Fixed. > + value = nm_setting_ip6_config_get_token (NM_SETTING_IP6_CONFIG (s_ip6)); > + if (value) > + svSetValue (ifcfg, "IPV6_TOKEN", value, FALSE); > > should be: > > + value = nm_setting_ip6_config_get_token (NM_SETTING_IP6_CONFIG (s_ip6)); > + svSetValue (ifcfg, "IPV6_TOKEN", value, FALSE); Fixed. > >> platform: add capability to set the tokenized interface identifier > > We don't need the token set in platform for out address mode generation, > ^^^ Fixed. > maybe _nl_msg_new_link_set_afspec() should not even nest_start(IFLA_AF_SPEC) > if there are no properties. > Add > > if (addr_gen_mode < 0 && !iid) > return TRUE; > > at the beginning. I don't think it ever gets called with no properties. > + _LOGD ("link: change %d: token: set IPv6 address generation token to > %s", > + ifindex, nm_utils_hwaddr_ntoa (&iid, sizeof (iid))); > > nm_utils_inet6_interface_identfier_to_token()? Fixed. > nm_platform_link_set_ipv6_token (NMPlatform *self, int ifindex, > NMUtilsIPv6IfaceId iid); > > > didn't you say, that the token cannot be set to "::"? Then, > nm_platform_link_set_ipv6_token() should have > > g_return_val_if_fail (iid.id, FALSE); Added. > and optionally, _nl_msg_new_link_set_afspec(), > > if (iid) { > nm_assert (iid->id); > ... I'm not sure it's necessary. It adds line noise and the platform would complain anyway. > >> device: set the iid to rdisc from connection's property > > - NMUtilsIPv6IfaceId iid; > + const NMPlatformLink *pllink; > > belongs to previous commit. Fixed. > nm_device_get_ip_iface_identifier() should only accept the token the > addr-gen-mode is EUI64. I don't think so. Why would it? > ip6_config_merge_and_apply() sets now the token every time. That doesn't > seem right. Is that a problem? It's consistent with e.g. setting MTU. > - if (!nm_device_get_ip_iface_identifier (self, &iid)) { > + /* Don't use nm_device_get_ip_iface_identifier() -- we don't want > to take > + * the token into account for the link-local address . No > particular good > + * reason for that other than that the kernel doesn't do that > either. */ > + if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, > &iid)) { > > > Could we instead add an argument "consider_setting" to > nm_device_get_ip_iface_identifier(). I find it confusing that there exist > direct calls to the virtual method which bypass the wrapper. Hmm. I thought the comment is perhaps sufficient for the single special case (which probably has no good reason to be a special case other that it matches the kernel implementation behavior). > if addrconf6_start_with_link_ready() fails to get the iid, I think it still > should call "nm_rdisc_set_iid(priv->rdisc, NULL);" to clear it. Why? (apart from that NULL is not valid there, since it's not a pointer). If anything goes wrong and the iid is not set even though it's needed then rdisc already deals with the error properly. > pushed a few commits. Thanks. Applied the fixups.
* 52fae4d - core-utils: add conversions of ipv6 tokens * 4f93d96 - platform: use utility functionality to stringify link order of commits should be swapped. (In reply to Lubomir Rintel from comment #4) > (In reply to Thomas Haller from comment #3) > > IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode. > > > > Documentation doesn't match behavior of verify(), also allows tokens with > > stable-privacy mode. > > I think it's correct? It only makes a statement on usefulness; you can set > the tokenized iid even when you don't use it to construct an address; but > that's not too useful at this point. Usually, verify() rejects settings that make no sense. E.g. you cannot set "master" if you don't have "slave-type". That doesn't mean, that in the future we couldn't relax the behavior and allow tokens for stable-privacy (if that is what you intend to do). > > nm_device_get_ip_iface_identifier() should only accept the token the > > addr-gen-mode is EUI64. > > I don't think so. Why would it? Related to previous point. verify() doesn't verify valid tokens in stable-privacy mode. So, it would be easy to configure a invalid token, which is attempted to be used. > > - if (!nm_device_get_ip_iface_identifier (self, &iid)) { > > + /* Don't use nm_device_get_ip_iface_identifier() -- we don't want > > to take > > + * the token into account for the link-local address . No > > particular good > > + * reason for that other than that the kernel doesn't do that > > either. */ > > + if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, > > &iid)) { > > > > > > Could we instead add an argument "consider_setting" to > > nm_device_get_ip_iface_identifier(). I find it confusing that there exist > > direct calls to the virtual method which bypass the wrapper. > > Hmm. I thought the comment is perhaps sufficient for the single special case > (which probably has no good reason to be a special case other that it > matches the kernel implementation behavior). Related to previous point. You do: if (s_ip6 && nm_setting_ip6_config_get_addr_gen_mode (s_ip6) == NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY) { ... } else { /* Don't use nm_device_get_ip_iface_identifier() -- we don't want to take * the token into account for the link-local address . No particular good * reason for that other than that the kernel doesn't do that either. */ if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid)) { ... } if nm_device_get_ip_iface_identifier() would ignore the token unless the mode is set to EUI64, you wouldn't need this hack at all. > > ip6_config_merge_and_apply() sets now the token every time. That doesn't > > seem right. > > Is that a problem? It's consistent with e.g. setting MTU. Maybe no problem... > > > if addrconf6_start_with_link_ready() fails to get the iid, I think it still > > should call "nm_rdisc_set_iid(priv->rdisc, NULL);" to clear it. > > Why? (apart from that NULL is not valid there, since it's not a pointer). say, you configure token ::53 and activate connection. Then, nmcli connection modify $CON ipv6.token "" nmcli device reapply $DEV would it properly clear the token? I don't know... Say, you start with no token or token ::42, then for some reason it changes and you do set the token in addrconf6_start_with_link_ready(). Ok, makes sense. But if the token changes to ::, you don't reset it. Why not? If you think that is right, it would be nice to add a comment why not clearing it.
(In reply to Thomas Haller from comment #5) > * 52fae4d - core-utils: add conversions of ipv6 tokens > * 4f93d96 - platform: use utility functionality to stringify link > > order of commits should be swapped. > > > (In reply to Lubomir Rintel from comment #4) > > (In reply to Thomas Haller from comment #3) > > > IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode. > > > > > > Documentation doesn't match behavior of verify(), also allows tokens with > > > stable-privacy mode. > > > > I think it's correct? It only makes a statement on usefulness; you can set > > the tokenized iid even when you don't use it to construct an address; but > > that's not too useful at this point. > > Usually, verify() rejects settings that make no sense. E.g. you cannot set > "master" if you don't have "slave-type". > That doesn't mean, that in the future we couldn't relax the behavior and > allow tokens for stable-privacy (if that is what you intend to do). Sounds sane; modified verify() to reject the tokens for non-eui64 modes now. > > > nm_device_get_ip_iface_identifier() should only accept the token the > > > addr-gen-mode is EUI64. > > > > I don't think so. Why would it? > > Related to previous point. > > verify() doesn't verify valid tokens in stable-privacy mode. So, it would be > easy to configure a invalid token, which is attempted to be used. > > > > > > - if (!nm_device_get_ip_iface_identifier (self, &iid)) { > > > + /* Don't use nm_device_get_ip_iface_identifier() -- we don't want > > > to take > > > + * the token into account for the link-local address . No > > > particular good > > > + * reason for that other than that the kernel doesn't do that > > > either. */ > > > + if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, > > > &iid)) { > > > > > > > > > Could we instead add an argument "consider_setting" to > > > nm_device_get_ip_iface_identifier(). I find it confusing that there exist > > > direct calls to the virtual method which bypass the wrapper. > > > > Hmm. I thought the comment is perhaps sufficient for the single special case > > (which probably has no good reason to be a special case other that it > > matches the kernel implementation behavior). > > > Related to previous point. > You do: > > if (s_ip6 && nm_setting_ip6_config_get_addr_gen_mode (s_ip6) == > NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY) { > ... > } else { > /* Don't use nm_device_get_ip_iface_identifier() -- we don't want to take > * the token into account for the link-local address . No particular good > * reason for that other than that the kernel doesn't do that either. */ > if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid)) { > ... > } > > if nm_device_get_ip_iface_identifier() would ignore the token unless the > mode is set to EUI64, you wouldn't need this hack at all. As per the in-person discussion; I've streamlined this differently (addition of an argument to ignore the token for the link-local special case). > > > if addrconf6_start_with_link_ready() fails to get the iid, I think it still > > > should call "nm_rdisc_set_iid(priv->rdisc, NULL);" to clear it. > > > > Why? (apart from that NULL is not valid there, since it's not a pointer). > > say, you configure token ::53 and activate connection. > Then, > nmcli connection modify $CON ipv6.token "" > nmcli device reapply $DEV > > would it properly clear the token? I don't know... > > Say, you start with no token or token ::42, (In reply to Thomas Haller from comment #5) > * 52fae4d - core-utils: add conversions of ipv6 tokens > * 4f93d96 - platform: use utility functionality to stringify link > > order of commits should be swapped. > > > (In reply to Lubomir Rintel from comment #4) > > (In reply to Thomas Haller from comment #3) > > > IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode. > > > > > > Documentation doesn't match behavior of verify(), also allows tokens with > > > stable-privacy mode. > > > > I think it's correct? It only makes a statement on usefulness; you can set > > the tokenized iid even when you don't use it to construct an address; but > > that's not too useful at this point. > > Usually, verify() rejects settings that make no sense. E.g. you cannot set > "master" if you don't have "slave-type". > That doesn't mean, that in the future we couldn't relax the behavior and > allow tokens for stable-privacy (if that is what you intend to do). Sounds sane; modified verify() to reject the tokens for non-eui64 modes now. > > > nm_device_get_ip_iface_identifier() should only accept the token the > > > addr-gen-mode is EUI64. > > > > I don't think so. Why would it? > > Related to previous point. > > verify() doesn't verify valid tokens in stable-privacy mode. So, it would be > easy to configure a invalid token, which is attempted to be used. > > > > > > - if (!nm_device_get_ip_iface_identifier (self, &iid)) { > > > + /* Don't use nm_device_get_ip_iface_identifier() -- we don't want > > > to take > > > + * the token into account for the link-local address . No > > > particular good > > > + * reason for that other than that the kernel doesn't do that > > > either. */ > > > + if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, > > > &iid)) { > > > > > > > > > Could we instead add an argument "consider_setting" to > > > nm_device_get_ip_iface_identifier(). I find it confusing that there exist > > > direct calls to the virtual method which bypass the wrapper. > > > > Hmm. I thought the comment is perhaps sufficient for the single special case > > (which probably has no good reason to be a special case other that it > > matches the kernel implementation behavior). > > > Related to previous point. > You do: > > if (s_ip6 && nm_setting_ip6_config_get_addr_gen_mode (s_ip6) == > NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY) { > ... > } else { > /* Don't use nm_device_get_ip_iface_identifier() -- we don't want to take > * the token into account for the link-local address . No particular good > * reason for that other than that the kernel doesn't do that either. */ > if (!NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid)) { > ... > } > > if nm_device_get_ip_iface_identifier() would ignore the token unless the > mode is set to EUI64, you wouldn't need this hack at all. As per the in-person discussion; I've streamlined this differently (addition of an argument to ignore the token for the link-local special case). > > > if addrconf6_start_with_link_ready() fails to get the iid, I think it still > > > should call "nm_rdisc_set_iid(priv->rdisc, NULL);" to clear it. > > > > Why? (apart from that NULL is not valid there, since it's not a pointer). > > say, you configure token ::53 and activate connection. > Then, > nmcli connection modify $CON ipv6.token "" > nmcli device reapply $DEV > > would it properly clear the token? I don't know... > > Say, you start with no token or token ::42, then for some reason it changes > and you do set the token in addrconf6_start_with_link_ready(). Ok, makes > sense. > But if the token changes to ::, you don't reset it. Why not? If you think > that is right, it would be nice to add a comment why not clearing it. It seems to me that on reapply the addrconf gets started anew; with new rdisc instance? Pushed a new version. then for some reason it changes > and you do set the token in addrconf6_start_with_link_ready(). Ok, makes > sense. > But if the token changes to ::, you don't reset it. Why not? If you think > that is right, it would be nice to add a comment why not clearing it. It seems to me that on reapply the addrconf gets started anew; with new rdisc instance? Pushed a new version.
>> setting-ip6-config: add token property libnm-core/nm-setting-ip6-config.c: + } + } whitespace^^ device: set the iid to rdisc from connection's property - if (!nm_device_get_ip_iface_identifier (self, &iid)) { + if (nm_device_get_ip_iface_identifier (self, &iid, TRUE)) { ^^^ ?? nm_device_get_ip_iface_identifier(): nm_utils_ipv6_interface_identfier_get_from_token() returning false should not happen because then the connection would not verify -- which would be a bug somewhere along the line. Still, could you better handle an invalid token, for example by fallback to self->get_ip_iface_identifier() or by asserting? Now, the token is only set when addr-gen-mode==EUI64. But in the future a tokens seems useful with addr-gen-mode!=EUI64. When we ever change that, nm_device_get_ip_iface_identifier() will wrongly take any token for the wrong addr-gen-mode. Will you remember to fix that when extending the token? Why not add a (nm_setting_ip6_config_get_token(s_ip6) == EUI64) check? > > and you do set the token in addrconf6_start_with_link_ready(). Ok, makes > > sense. > > But if the token changes to ::, you don't reset it. Why not? If you think > > that is right, it would be nice to add a comment why not clearing it. > It seems to me that on reapply the addrconf gets started anew; with new rdisc > instance? Sorry, the point was not what happens on reapply (I was confusing here). The point is, a failure to lookup a token means for every practical purpose that there is no token. Why not set rdisc's token to :: in that case too? Why treat a failure special and not touching rdisc's iid? If that is intended, then this special behavior warrants a better comment. There are comments ("if rdisc needs the iid it will notice this itself"), but it's not clear to me still. Why not: NMUtilsIPv6IfaceId iid = {0}; if (!nm_device_get_ip_iface_identifier (self, &iid, FALSE)) { _LOGI (LOGD_IP6, "addrconf6: no interface identifier; IPv6... memset (&iid, 0, sizeof (iid)); } else { _LOGD (LOGD_IP6, "addrconf6: using the device EUI-64 identifier: %s", _to_string(iid)); } nm_rdisc_set_iid (priv->rdisc, iid); (the latter doesn't need a comment, because for me that is what I would expect).
> >> setting-ip6-config: add token property > > libnm-core/nm-setting-ip6-config.c: > + } > + } > whitespace^^ Fixed. > device: set the iid to rdisc from connection's property > > - if (!nm_device_get_ip_iface_identifier (self, &iid)) { > + if (nm_device_get_ip_iface_identifier (self, &iid, TRUE)) { > ^^^ ?? Whoops. Fixed. > nm_device_get_ip_iface_identifier(): > > nm_utils_ipv6_interface_identfier_get_from_token() returning false should not happen because then the connection would not verify -- which would be a bug somewhere along the line. > Still, could you better handle an invalid token, for example by fallback to self->get_ip_iface_identifier() or by asserting? I think that it's not worth making the code more complex for a can-no-happen scenario here. If that happens we'll end up with no iid which will make rdisc fail -- i believe that's good enough already. > Now, the token is only set when addr-gen-mode==EUI64. But in the future a tokens seems useful with addr-gen-mode!=EUI64. When we ever change that, nm_device_get_ip_iface_identifier() will wrongly take any token for the wrong addr-gen-mode. Will you remember to fix that when extending the token? > Why not add a > (nm_setting_ip6_config_get_token(s_ip6) == EUI64) > check? I don't know. Extra line noise for stuff that doesn't actually exist sounds like a bad thing to me. > > > and you do set the token in addrconf6_start_with_link_ready(). Ok, makes > > > sense. > > > But if the token changes to ::, you don't reset it. Why not? If you think > > > that is right, it would be nice to add a comment why not clearing it. > > > It seems to me that on reapply the addrconf gets started anew; with new rdisc > > instance? > > Sorry, the point was not what happens on reapply (I was confusing here). The point is, a failure to lookup a token means for every practical purpose that there is no token. Why not set rdisc's token to :: in that case too? Why treat a failure special and not touching rdisc's iid? Rdisc has no token. You're probably confusing that with the iid. I can't see why do you believe it's a special case. No iid is a pretty good indication iid could not be determined and it's up to rdisc to decide if that's a bad thing or not. > > If that is intended, then this special behavior warrants a better comment. There are comments ("if rdisc needs the iid it will notice this itself"), but it's not clear to me still. > > Why not: > > NMUtilsIPv6IfaceId iid = {0}; > > if (!nm_device_get_ip_iface_identifier (self, &iid, FALSE)) { > _LOGI (LOGD_IP6, "addrconf6: no interface identifier; IPv6... > memset (&iid, 0, sizeof (iid)); > } else { > _LOGD (LOGD_IP6, "addrconf6: using the device EUI-64 identifier: %s", > _to_string(iid)); > } > nm_rdisc_set_iid (priv->rdisc, iid); > > > (the latter doesn't need a comment, because for me that is what I would expect). Updated the branch.
> core-utils: add conversions of ipv6 tokens +nm_utils_ipv6_interface_identfier_get_from_token (NMUtilsIPv6IfaceId *iid, +nm_utils_inet6_interface_identfier_to_token (NMUtilsIPv6IfaceId iid, char *buf) s/identfier/identifier/ ? > device: use the token set in platform when generating a connection devices/nm-device.c: In function ‘nm_device_generate_connection’: devices/nm-device.c:3149:3: error: ‘s_ip6’ may be used uninitialized in this function [-Werror=maybe-uninitialized] g_object_set (s_ip6, Can you initialize s_ip6 to NULL and add a check before using it? The rest LGTM. It would be useful to have nmcli support too, do you plan to add it later?
(In reply to Beniamino Galvani from comment #9) > > core-utils: add conversions of ipv6 tokens > > +nm_utils_ipv6_interface_identfier_get_from_token (NMUtilsIPv6IfaceId *iid, > +nm_utils_inet6_interface_identfier_to_token (NMUtilsIPv6IfaceId iid, char > *buf) > > s/identfier/identifier/ ? Huh. Fixed. > > device: use the token set in platform when generating a connection > > devices/nm-device.c: In function ‘nm_device_generate_connection’: > devices/nm-device.c:3149:3: error: ‘s_ip6’ may be used uninitialized in > this function [-Werror=maybe-uninitialized] > g_object_set (s_ip6, > > Can you initialize s_ip6 to NULL and add a check before using it? Fixed. > The rest LGTM. It would be useful to have nmcli support too, do you plan > to add it later? Dropped by accident. Added now back.
> platform: remove unnecessary NMPlatformLink.inet6_token.is_valid field I probably don't understand what's going on here, but it looks like the kernel will *always* send IFLA_INET6_TOKEN even if it's not actually set for the interface. So wouldn't this patch say there is a valid token when there actually isn't a valid token? inet6_fill_ifla6_attrs(): nla = nla_reserve(skb, IFLA_INET6_TOKEN, sizeof(struct in6_addr)); if (!nla) goto nla_put_failure; if (nla_put_u8(skb, IFLA_INET6_ADDR_GEN_MODE, idev->addr_gen_mode)) goto nla_put_failure; read_lock_bh(&idev->lock); ---> memcpy(nla_data(nla), idev->token.s6_addr, nla_len(nla)); read_unlock_bh(&idev->lock); > platform: use utility functionality to stringify link "identfier" -> identifier since that was already fixed in a previous patch, I think this patch will no longer build. > setting-ip6-config: add token property + case PROP_TOKEN: + g_free (priv->token); + priv->token = g_strdup (g_value_get_string (value)); g_value_dup_string(value)? Also, this patch has the identfier -> identifier fixup that should go into the previous patch. Rest LGTM.
Merged
for the record: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2edaf35f1296b4b8f140365b4b40d5d89e57128a