GNOME Bugzilla – Bug 773777
[review] lr/ipv6-sharing: support IPv6 connection sharing via DHCP6 prefix delegation
Last modified: 2016-11-09 16:38:48 UTC
https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/ipv6-sharing
>> linux: don't assume short write when the kernel ignores the trailing whitespace - } else if (nwrote < len) { + } else if (nwrote < strlen (value)) { above we already calculate strlen(value). It is: "len - 1". Note above: for (tries = 0, nwrote = 0; tries < 3 && nwrote != len; tries++) { ^^^^^^^^^^^^^ >> ifcfg-rh/trivial: get rid of an extra unused variable this doesn't look "trivial" in the sense as I understand the word. Can we drop that word from the commit message? + str_value = svGetValue (ifcfg, "IPV6_PRIVACY", FALSE); + if (str_value) { ip6_privacy = svGetValueBoolean (ifcfg, "IPV6_PRIVACY", FALSE); avoid double lookup of the variable. As you already have the string-representation, use ip6_privacy = svParseBoolean (str_value, FALSE); (more tomorrow, looks nice!)
> device: avoid a crash when L3 configuration fails fw_change_zone_cb_ip_check() is called in _set_state_full(IP_CHECK) and so I assume the only case in which this situation can be hit is when both IPv4 and IPv6 fail (or one fails and the other is disabled) and the connection is assumed (line 4225 of nm-device.c). In _set_state_full(IP_CHECK) we call fw_change_zone_cb_ip_check() only when (priv->ifindex != priv->ip_ifindex), while for other devices nm_device_start_ip_check() is called directly. The patch seems correct to me, but shouldn't we do the same check also when nm_device_start_ip_check() is called directly (line 11715)? > ndisc: add logic for acting as a router NMNDisc * nm_fake_ndisc_new (int ifindex, const char *ifname) { return g_object_new (NM_TYPE_FAKE_NDISC, NM_NDISC_IFINDEX, ifindex, NM_NDISC_IFNAME, ifname, + NM_NDISC_NODE_TYPE, NM_NDISC_NODE_TYPE_HOST, NM_NDISC_NODE_TYPE, (int) NM_NDISC_NODE_TYPE_HOST ? +static void +solicited_router_announce (NMNDisc *ndisc) +{ + [...] + if (!priv->send_ra_id) { + priv->send_ra_id = g_timeout_add (g_random_int_range (0, 500), Can you use the symbolic name here too (MAX_RA_DELAY_TIME)? + /* Schedule next initial announcement retransmit. */ + priv->send_ra_id = g_timeout_add_seconds (g_random_int_range (0, NDP_MAX_INITIAL_RTR_ADVERT_INTERVAL), + (GSourceFunc) announce_router, ndisc); I think we should ratelimit to at most one announcement every MIN_DELAY_BETWEEN_RAS seconds. > device: add IPv6 configuration delegation machinery I have seen this: NetworkManager[29236]: <debug> [1478079050.1731] device[0x17d66f0] (enp0s25): ipv6-pd: asking DHCP6 for 1 prefixes NetworkManager[29236]: nm_device_dhcp6_renew: assertion 'priv->dhcp6.client != NULL' failed
+ Trace 236806
(In reply to Thomas Haller from comment #1) > >> linux: don't assume short write when the kernel ignores the trailing whitespace > > > - } else if (nwrote < len) { > + } else if (nwrote < strlen (value)) { > > above we already calculate strlen(value). It is: "len - 1". Uh, ok. I wrongly assumed someone may call this with "\n" already in the string. Replaced the strlen() with len - 1, though I still find that sort of hard to follow. Not a big deal though. > Note above: > > for (tries = 0, nwrote = 0; tries < 3 && nwrote != len; tries++) { > ^^^^^^^^^^^^^ Fixed. > >> ifcfg-rh/trivial: get rid of an extra unused variable > > this doesn't look "trivial" in the sense as I understand the word. Can we > drop that word from the commit message? Okay. > + str_value = svGetValue (ifcfg, "IPV6_PRIVACY", FALSE); > + if (str_value) { > ip6_privacy = svGetValueBoolean (ifcfg, "IPV6_PRIVACY", FALSE); > > avoid double lookup of the variable. As you already have the > string-representation, use > > ip6_privacy = svParseBoolean (str_value, FALSE); Added a commit to address that. (In reply to Beniamino Galvani from comment #2) > > device: avoid a crash when L3 configuration fails > > fw_change_zone_cb_ip_check() is called in _set_state_full(IP_CHECK) > and so I assume the only case in which this situation can be hit is > when both IPv4 and IPv6 fail (or one fails and the other is disabled) > and the connection is assumed (line 4225 of nm-device.c). > > In _set_state_full(IP_CHECK) we call fw_change_zone_cb_ip_check() only > when (priv->ifindex != priv->ip_ifindex), while for other devices > nm_device_start_ip_check() is called directly. > > The patch seems correct to me, but shouldn't we do the same check also > when nm_device_start_ip_check() is called directly (line 11715)? I think we never progress to the IP_CHECK state until either v4 of v6 is DONE? > > ndisc: add logic for acting as a router > > NMNDisc * > nm_fake_ndisc_new (int ifindex, const char *ifname) > { > return g_object_new (NM_TYPE_FAKE_NDISC, > NM_NDISC_IFINDEX, ifindex, > NM_NDISC_IFNAME, ifname, > + NM_NDISC_NODE_TYPE, NM_NDISC_NODE_TYPE_HOST, > > NM_NDISC_NODE_TYPE, (int) NM_NDISC_NODE_TYPE_HOST ? Why? I think the compiler coerces this enum to an unsigned int, which is cast into an int by default? Does your compiler complain about this? > +static void > +solicited_router_announce (NMNDisc *ndisc) > +{ > + [...] > + if (!priv->send_ra_id) { > + priv->send_ra_id = g_timeout_add (g_random_int_range (0, > 500), > > Can you use the symbolic name here too (MAX_RA_DELAY_TIME)? Done. > + /* Schedule next initial announcement retransmit. */ > + priv->send_ra_id = g_timeout_add_seconds (g_random_int_range > (0, NDP_MAX_INITIAL_RTR_ADVERT_INTERVAL), > + (GSourceFunc) > announce_router, ndisc); > > I think we should ratelimit to at most one announcement every > MIN_DELAY_BETWEEN_RAS seconds. We do. The initial interval is always larged than the min delay and if there would be another solicitation scheduled before this one is sent, it would first just unschedule this one. > > device: add IPv6 configuration delegation machinery > > I have seen this: > > NetworkManager[29236]: <debug> [1478079050.1731] device[0x17d66f0] > (enp0s25): ipv6-pd: asking DHCP6 for 1 prefixes > NetworkManager[29236]: nm_device_dhcp6_renew: assertion 'priv->dhcp6.client > != NULL' failed Fixed. Pushed an updated version.
(In reply to Lubomir Rintel from comment #3) > > NM_NDISC_NODE_TYPE, (int) NM_NDISC_NODE_TYPE_HOST ? > > Why? I think the compiler coerces this enum to an unsigned int, which is > cast into an int by default? Does your compiler complain about this? I think it should always be correct without cast. Ahe cast makes it explict and avoid the uncertainty. So I actually prefer explicit casts of enums as variadic arguments.
> > NM_NDISC_NODE_TYPE, (int) NM_NDISC_NODE_TYPE_HOST ? > > Why? I think the compiler coerces this enum to an unsigned int, which > is cast into an int by default? Does your compiler complain about > this? You're right, I thought that the enum could be larger than an int, but that is not possible. > > The patch seems correct to me, but shouldn't we do the same check also > > when nm_device_start_ip_check() is called directly (line 11715)? > > I think we never progress to the IP_CHECK state until either v4 of v6 is DONE? I think that if this was the case, the patch shouldn't be needed since fw_change_zone_cb_ip_check() is called only from the IP_CHECK case of _set_state_full(). Actually, we can progress to IP_CHECK when both (ip4_state/ip6_state == FAIL) and the connection is assumed. So in my opinion the same check that you add to fw_change_zone_cb_ip_check() should be added also before calling nm_device_start_ip_check() in _set_state_full(IP_CHECK). >> I think we should ratelimit to at most one announcement every >> MIN_DELAY_BETWEEN_RAS seconds. > We do. The initial interval is always larged than the min delay and > if there would be another solicitation scheduled before this one is > sent, it would first just unschedule this one. Maybe I'm reading the code wrong, but here we set a timer with a delay that can be 0: + if (--priv->announcements_left) { + _LOGD ("will resend an initial router advertisement"); + + /* Schedule next initial announcement retransmit. */ + priv->send_ra_id = g_timeout_add_seconds (g_random_int_range (0, NM_NDISC_ROUTER_ADVERT_INITIAL_INTERVAL), + (GSourceFunc) announce_router, ndisc); and the next announcement will be sent as soon as the function ends. > policy: delegate IPv6 configuration to ipv6.method=shared connections +static gboolean +ip6_subnet_from_delegation (IP6PrefixDelegation *delegation, NMDevice *device) +{ + NMPlatformIP6Address *subnet; + int ifindex = nm_device_get_ifindex (device); + + subnet = g_hash_table_lookup (delegation->subnets, GINT_TO_POINTER (ifindex)); + if (!subnet) { + /* Check for out-of-prefixes condition. */ + if (delegation->next_subnet >= 1 << (64 - delegation->prefix.plen)) { Can you please add parenthesis around the bitshift, so that I don't have to check 'man operator' every time I look at it? :) + /* Allocate a new subnet. */ + subnet = g_slice_new0 (NMPlatformIP6Address); + g_hash_table_insert (delegation->subnets, GINT_TO_POINTER (ifindex), subnet); It seems subnets are never freed. +static·void +expire_ip6_delegations·(NMPolicy·*self) +{ +» NMPolicyPrivate·*priv·=·NM_POLICY_GET_PRIVATE·(self); +» guint32·now·=·nm_utils_get_monotonic_timestamp_s·(); +········IP6PrefixDelegation·*delegation·=·NULL; +» int·i; + +» for·(i·=·0;·i·<·priv->ip6_prefix_delegations->len;·i++)·{ +················delegation·=·&g_array_index·(priv->ip6_prefix_delegations, +» » ·····························IP6PrefixDelegation,·i); Indentation.
(In reply to Beniamino Galvani from comment #5) > > > The patch seems correct to me, but shouldn't we do the same check also > > > when nm_device_start_ip_check() is called directly (line 11715)? > > > > I think we never progress to the IP_CHECK state until either v4 of v6 is DONE? > > I think that if this was the case, the patch shouldn't be needed since > fw_change_zone_cb_ip_check() is called only from the IP_CHECK case of > _set_state_full(). > > Actually, we can progress to IP_CHECK when both (ip4_state/ip6_state > == FAIL) and the connection is assumed. So in my opinion the same > check that you add to fw_change_zone_cb_ip_check() should be added > also before calling nm_device_start_ip_check() in _set_state_full(IP_CHECK). The difference is that if we call nm_device_start_ip_check() from a callback to zone change rather than right after the _set_state_full(IP_CHECK) is that the device state can change while the zone change is mid-air. I think in the particular case where I've seen the crash it was when IPv4 sharing failed. That happens in stage5, after the stage3 returns success, commits config and transitions the device to IP_CHECK. It could be something like this: * Let's assume ip6_status = ip4_status = IP_WAIT (or anything that's not IP_DONE) * IPv4 stage3 succeeds, sets ip4_status = IP_DONE * device transitions to NM_DEVICE_STATE_IP_CHECK * Call to nm_firewall_manager_add_or_change_zone () defers the actual config check * IPv4 stage5 fails IPv4, ip4_status = FAIL * the deferred config_check takes place, but the configuration is now failed Thus we need to check for changes in IP statuses in deferred checks, but not when they're triggered directly from _set_state_full(). > >> I think we should ratelimit to at most one announcement every > >> MIN_DELAY_BETWEEN_RAS seconds. > > > We do. The initial interval is always larged than the min delay and > > if there would be another solicitation scheduled before this one is > > sent, it would first just unschedule this one. > > Maybe I'm reading the code wrong, but here we set a timer with a delay > that can be 0: > > + if (--priv->announcements_left) { > + _LOGD ("will resend an initial router advertisement"); > + > + /* Schedule next initial announcement retransmit. */ > + priv->send_ra_id = g_timeout_add_seconds (g_random_int_range > (0, NM_NDISC_ROUTER_ADVERT_INITIAL_INTERVAL), > + (GSourceFunc) > announce_router, ndisc); > > and the next announcement will be sent as soon as the function ends. You're reading that right -- I was looking in the wrong place first. Added a fixup. > > policy: delegate IPv6 configuration to ipv6.method=shared connections > > +static gboolean > +ip6_subnet_from_delegation (IP6PrefixDelegation *delegation, NMDevice > *device) > +{ > + NMPlatformIP6Address *subnet; > + int ifindex = nm_device_get_ifindex (device); > + > + subnet = g_hash_table_lookup (delegation->subnets, GINT_TO_POINTER > (ifindex)); > + if (!subnet) { > + /* Check for out-of-prefixes condition. */ > + if (delegation->next_subnet >= 1 << (64 - > delegation->prefix.plen)) { > > Can you please add parenthesis around the bitshift, so that I don't > have to check 'man operator' every time I look at it? :) Added a fixup. > + /* Allocate a new subnet. */ > + subnet = g_slice_new0 (NMPlatformIP6Address); > + g_hash_table_insert (delegation->subnets, GINT_TO_POINTER > (ifindex), subnet); > > It seems subnets are never freed. Uh, right. Added a fixup. > +static·void > +expire_ip6_delegations·(NMPolicy·*self) > +{ > +» NMPolicyPrivate·*priv·=·NM_POLICY_GET_PRIVATE·(self); > +» guint32·now·=·nm_utils_get_monotonic_timestamp_s·(); > +········IP6PrefixDelegation·*delegation·=·NULL; > +» int·i; > + > +» for·(i·=·0;·i·<·priv->ip6_prefix_delegations->len;·i++)·{ > +················delegation·=·&g_array_index·(priv->ip6_prefix_delegations, > +» » ·····························IP6PrefixDelegation,·i); > > Indentation. Added a fixup. Updated the branch.
if (!ip6_privacy) - ip6_privacy = g_strcmp0 (tmp, "rfc4941") == 0 || - g_strcmp0 (tmp, "rfc3041") == 0; + ip6_privacy = g_strcmp0 (str_value, "rfc4941") == 0 || + g_strcmp0 (str_value, "rfc3041") == 0; let's fix missing braces. Or just use NM_IN_STRSET (str_value, "rfc4941", "rfc3041") >> device: avoid a crash when L3 configuration fails while at it, use g_return_if_fail() in nm_device_start_ip_check() »···g_assert (!priv->gw_ping.watch); »···g_assert (!priv->gw_ping.timeout); »···g_assert (!priv->gw_ping.pid); »···g_assert (priv->ip4_state == IP_DONE || priv->ip6_state == IP_DONE); >> ndisc: add a couple of comments of where do the constants come from trivial? >> ndisc: avoid calling start() multiple times + g_assert (!priv->event_channel); + g_assert (!priv->event_id); g_assert (klass->start); + g_assert (!priv->ra_timeout_id); g_return_if_fail()? - if (nm_ndisc_set_iid (priv->ndisc, info.inet6_token)) { + if (nm_ndisc_set_iid (priv->ndisc, info.inet6_token)) _LOGD (LOGD_DEVICE, "IPv6 tokenized identifier present on device %s", priv->iface); - nm_ndisc_start (priv->ndisc); - } or maybe nm_ndisc_start() should just be idempotent instead? Are we sure that ndisc is already started? Non-obvious. >> ndisc: move the logging deduplication into a macro many _NMLOG() macros require an implicit @self variable to be around. _MAYBE_WARN() also requires @priv. Avoid that by using _GET_PRIVATE(self) instead (_GET_PRIVATE() is really just a pointer de-reference, so there is virtually no performance penalty) _MAYBE_WARN() also requires an implict "error" argument which is unexpected. I think that is quite ugly, and there should be an error argument instead. And preferably cache @error in a local variable, to evaluate it exactly once. >> ndisc: add logic for acting as a router + NM_NDISC_NODE_TYPE, NM_NDISC_NODE_TYPE_HOST, (int) + NM_NDISC_NODE_TYPE_ROUTER +} NMNDiscNodeType; add trailing , +nm_ndisc_set_config (NMNDisc *ndisc, const NMNDiscData *rdata) NMNDiscData is really the data structure with all the fields received from an RA. I think it's the wrong data-type to configure what the router should announce. I think instead, there should be just the arguments that are needed. For example, avoid: + data.dns_domains = (NMNDiscDNSDomain *)g_array_free (array, FALSE); + + nm_ndisc_set_config (ndisc, &data); + g_free ((gpointer) data.addresses); + g_free ((gpointer) data.dns_servers); + g_free ((gpointer) data.dns_domains); + case PROP_NODE_TYPE: + /* construct-only */ + priv->node_type = g_value_get_int (value); + break; g_return_val_if_fail (priv->node_type != UNKNOWN); get the assertion where the error happens, not later during nm_ndisc_start(): + default: + g_assert_not_reached (); + } gint32 solicitations_left; + gint32 announcements_left; guint send_rs_id; + guint send_ra_id; gint32 last_rs; + gint32 last_ra; seems the fields are never used at the same time. Pack them in a union? Equally, rdata using for sending and receiving is wrong. Pack them also in a union, and have them have the proper fields that make sense for the current mode. +initial_router_announce (NMNDisc *ndisc) +solicited_router_announce (NMNDisc *ndisc) the two modes are partly exclusive. Can we reflect in the function names which belong to RS mode and which to RA? Eg, we have solicit_routers(), why are these not called announce_initial() and announce_solicited()?
(In reply to Thomas Haller from comment #7) > if (!ip6_privacy) > - ip6_privacy = g_strcmp0 (tmp, "rfc4941") == 0 || > - g_strcmp0 (tmp, "rfc3041") == 0; > + ip6_privacy = g_strcmp0 (str_value, "rfc4941") == 0 || > + g_strcmp0 (str_value, "rfc3041") == 0; > > > let's fix missing braces. Or just use > NM_IN_STRSET (str_value, "rfc4941", "rfc3041") Ok. > >> device: avoid a crash when L3 configuration fails > > while at it, use g_return_if_fail() in nm_device_start_ip_check() > > »···g_assert (!priv->gw_ping.watch); > »···g_assert (!priv->gw_ping.timeout); > »···g_assert (!priv->gw_ping.pid); > »···g_assert (priv->ip4_state == IP_DONE || priv->ip6_state == IP_DONE); Ok. > >> ndisc: add a couple of comments of where do the constants come from > > trivial? Ok. > >> ndisc: avoid calling start() multiple times > > + g_assert (!priv->event_channel); > + g_assert (!priv->event_id); > > g_assert (klass->start); > + g_assert (!priv->ra_timeout_id); > > g_return_if_fail()? Ok. > - if (nm_ndisc_set_iid (priv->ndisc, info.inet6_token)) { > + if (nm_ndisc_set_iid (priv->ndisc, info.inet6_token)) > _LOGD (LOGD_DEVICE, "IPv6 tokenized identifier present on > device %s", priv->iface); > - nm_ndisc_start (priv->ndisc); > - } > > or maybe nm_ndisc_start() should just be idempotent instead? Are we sure > that ndisc is already started? Non-obvious. Not sure. Perhaps a matter of personal taste. This seems more straightforward to me. > >> ndisc: move the logging deduplication into a macro > > many _NMLOG() macros require an implicit @self variable to be around. > _MAYBE_WARN() also requires @priv. Avoid that by using _GET_PRIVATE(self) > instead (_GET_PRIVATE() is really just a pointer de-reference, so there is > virtually no performance penalty) Not sure if that's worthwhile doing. This, given its assumptions about surrounding code (e.g. that it handles last_error) make this really not an universal macro and it seems alright to make more arbitrary assumptions (such as priv in scope) to keep it concise. To be honest I have a strong distaste for a stateful logging macro that and I'd perfer to remove it altogether. But that would, of course, reintroduce the annoyance it was meant to fix. I just believe rate limiting should *not* be done here and it's the logging daemon's job. If the user is not interested in the message or it's not really a problem to communicate them we maybe should always log at the debug level... > _MAYBE_WARN() also requires an implict "error" argument which is unexpected. > I think that is quite ugly, and there should be an error argument instead. > And preferably cache @error in a local variable, to evaluate it exactly once. The same reasoning as above. If we really want to do rate-limiting sensibly (I certainly don't), then we better implement something like printk_ratelimited(). Otherwise I think a patchy macro such as this one is sort of okay. > >> ndisc: add logic for acting as a router > > + NM_NDISC_NODE_TYPE, NM_NDISC_NODE_TYPE_HOST, > > (int) > > > + NM_NDISC_NODE_TYPE_ROUTER > +} NMNDiscNodeType; > > add trailing , Ok. > +nm_ndisc_set_config (NMNDisc *ndisc, const NMNDiscData *rdata) > > NMNDiscData is really the data structure with all the fields received from > an RA. I disagree at this point. It's the data associated with a NDisc. Whether you're in the host mode or in the router mode you operate on the exact same data set -- the only difference is whether you're announcing stuff or you discovered it. > I think it's the wrong data-type to configure what the router should > announce. I think instead, there should be just the arguments that are > needed. > For example, avoid: > + data.dns_domains = (NMNDiscDNSDomain *)g_array_free (array, FALSE); > + > + nm_ndisc_set_config (ndisc, &data); > + g_free ((gpointer) data.addresses); > + g_free ((gpointer) data.dns_servers); > + g_free ((gpointer) data.dns_domains); It was not me, it was this ugly before. :/ NDisc uses a more sensible structure (NMNDiscDataInternal) internally and NMDiscData is used only to communicate with the NMDevice. The translation to NMDevice is criminally ugly too -- maybe we could use NMNDiscDataInternal externally too and replace NMNDiscData with it? > + case PROP_NODE_TYPE: > + /* construct-only */ > + priv->node_type = g_value_get_int (value); > + break; > > g_return_val_if_fail (priv->node_type != UNKNOWN); > get the assertion where the error happens, not later during nm_ndisc_start(): > + default: > + g_assert_not_reached (); > + } But that ruins the code feng-shui. No, really, 'node_type != UNKNOWN' is not really equivalent to the default branch of the case. Nor am I convinced that it makes sense to do extensive checks on setting the property -- the assert in the case is merely there because it requires little effort to catch the exceptional situation and a default branch in case is usually a good coding style anyway. > gint32 solicitations_left; > + gint32 announcements_left; > guint send_rs_id; > + guint send_ra_id; > gint32 last_rs; > + gint32 last_ra; > > seems the fields are never used at the same time. Pack them in a union? Okay. > Equally, rdata using for sending and receiving is wrong. Pack them also in a > union, and have them have the proper fields that make sense for the current > mode. I disagree here -- the rdata is the ndisc configuration no matter what mode are we in. The same data, we just vary on the detail whether we're receiving or sending it. > +initial_router_announce (NMNDisc *ndisc) > +solicited_router_announce (NMNDisc *ndisc) > > the two modes are partly exclusive. Can we reflect in the function names > which belong to RS mode and which to RA? > Eg, we have solicit_routers(), why are these not called announce_initial() > and announce_solicited()? Hmm, I think the "router" word belongs there too, for consistency and also to accurately describe what the function actually does. Nevertheless "solicited_router_announce()" read a bit more naturally to me than "announce_router_solicited()". But anyway; I think you have a good point renaming it (but keeping the "router" word).
Pushed an updated branch.
nm_ndisc_set_config (NMNDisc *ndisc, const NMNDiscData *rdata) doesn't care about the other fields that are relevant for client mode (dhcp_level, mtu, hop_limit, gateways, routes). Do you think these properties will make sense later on? NMNDiscData as is is there to pack all arguments together when emitting the RA-received-signal. I guess that makes some sense because you can pass this pack around as one (instead of having to pass around 8 different parameters). That doesn't mean that it's a good idea to pack the configuration data in server mode too. Or to reuse the same NMNDiscData structure to do that. There are really just 3 arguments. Why not: nm_ndisc_set_config (rdisc, addresses->data, addresses->len, dns_servers->data, dns_servers->len, dns_domains->data, dns_domains->len); It seems now "config-changed" signal should be renamed (because it has no significance in server-mode. anyway, it's only about the naming. If you think it makes sense as-is...
(In reply to Thomas Haller from comment #10) > nm_ndisc_set_config (NMNDisc *ndisc, const NMNDiscData *rdata) > > doesn't care about the other fields that are relevant for client mode > (dhcp_level, mtu, hop_limit, gateways, routes). > Do you think these properties will make sense later on? > > > NMNDiscData as is is there to pack all arguments together when emitting the > RA-received-signal. > I guess that makes some sense because you can pass this pack around as one > (instead of having to pass around 8 different parameters). > > That doesn't mean that it's a good idea to pack the configuration data in > server mode too. Or to reuse the same NMNDiscData structure to do that. > > > There are really just 3 arguments. Why not: > > nm_ndisc_set_config (rdisc, > addresses->data, addresses->len, > dns_servers->data, dns_servers->len, > dns_domains->data, dns_domains->len); That sounds reasonable. Done. > It seems now "config-changed" signal should be renamed (because it has no > significance in server-mode. Agreed. Changed. Updated, pushed, merged.