GNOME Bugzilla – Bug 732965
[review] dcbw/dhcp-cleanups: DHCP cleanups and preparation for library-based clients
Last modified: 2014-07-23 15:00:35 UTC
This branch removes assumptions that DHCP clients will be child process based, and that all updates for DHCP events will come through the child process.
>> dhcp: simplify DHCP states DHCP_STATE_DONE, /* END */ - DHCP_STATE_FAIL /* failed or quit unexpectedly */ + DHCP_STATE_FAIL, /* failed or quit unexpectedly */ } NMDhcpState; possibly add __DHCP_STATE_MAX, DHCP_STATE_MAX = __DHCP_STATE_MAX - 1, and static const char *state_table[DHCP_STATE_MAX + 1] = { >> core: consolidate DHCP client termination in dhcp[4|6]_fail() + dhcp6_cleanup (self, TRUE, FALSE); has trailing white space >> dhcp: remove the client REMOVE signal The 5 second removal signal timeout appears to no longer be necessary, since the running child process is killed synchronously. Can this be changed to kill the child async? For some reason dhclient often refuses to terminate immediately on SIGTERM, blocking NM for seconds. btw, I merged th/kill_child. it logs now how long it takes to kill dhclient (0.5 seconds) I think the proper solution would be that nm-dhcp-manager kills the clinet async, but it pretends that the dhclient is already gone. If somebody else happens to start another dhcp on the same interface, manager should delay the start until to to-be-killed instances is really gone. But that sounds complex. >> dhcp: pass IP config in state signal indention in nm_dhcp_client_set_state() how about adding: g_return_if_fail (!ip6_config || NM_IS_IP6_CONFIG (ip6_config)) to dhcp6_state_changed? v4 respectively. >> dhcp: filter DHCP options when setting them + static const char *ignored_options[] = { + "interface", "pid", "reason", "dhcp_message_type", NULL + }; How about each option on a separate line, which is easier to read and results in nicer diffs when adding options later. + str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); + if (str_value) { + const char *tmp = str_value; + + if (g_str_has_prefix (tmp, NEW_TAG)) + tmp += strlen (NEW_TAG); + g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); + } this seems wrong. tmp is the "value", I think the key should be prefixed by NEW_TAG. also: + /* Filter out stuff that's not actually new DHCP options */ + for (p = ignored_options; *p; p++) { + if (!strcmp (*p, key)) + return; + } don't we care here about the NEW_TAG/OLD_TAG prefix? >> dhcp: consolidate more state logic in nm_dhcp_client_set_state() would be nice to split the commit in a trivial first, that just moves code around. >> dhcp: pass IPv6 privacy preference to client class nm_dhcp_manager_start_ip4() - timeout, dhcp_anycast_addr, hostname, FALSE, 0); + timeout, dhcp_anycast_addr, hostname, FALSE, NM_SETTING_IP6_CONFIG_PRIVACY_UNKNOWN); finally: could you modify nm_dhcp_manager_start_ip6() not to take a NMSetting, instead pass the needed parameters as is? I.e. privacy and hostname. Same for v4. This decouples NMDHCPx from NMSetting. rest looks good.
btw. I like the cleanups... fixes things that already bothered me.
(In reply to comment #1) > >> dhcp: filter DHCP options when setting them > + str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); > + if (str_value) { > + const char *tmp = str_value; > + > + if (g_str_has_prefix (tmp, NEW_TAG)) > + tmp += strlen (NEW_TAG); > + g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); > + } > > this seems wrong. tmp is the "value", I think the key should be prefixed by > NEW_TAG. The keys do not get prefixed by NEW_TAG at all, and none of the OLD_TAG keys are exposed to clients. So while dhclient sends us options with "new_ip_address" and "old_ip_address", clients have only ever seen "ip_address". eg, the API of NMDHCP4Config/NMDHCP6Config is that they only ever expose the *new* options. Thus the "new_" is redundant, and is stripped off. The only reason to expose this stuff anyway is to allow scripts to handle DHCP options that NM does not handle, like NIS servers and the NIS domain, or SIP servers, or stuff like that. > also: > > + /* Filter out stuff that's not actually new DHCP options */ > + for (p = ignored_options; *p; p++) { > + if (!strcmp (*p, key)) > + return; > + } > > don't we care here about the NEW_TAG/OLD_TAG prefix? No, we don't. Only new options are exposed to clients, and the "new_" prefix is stripped because it's redundant, as described above. Previously, the code kept all options (even the old ones) and only filtered them/stripped the names when adding to the NMDHCPxConfig object. But since we never use the old options, we might as well save some memory and just immediately discard the options we never need or expose to clients. Which is what this code is trying to do.
(In reply to comment #3) > (In reply to comment #1) > > >> dhcp: filter DHCP options when setting them > > + str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); > > + if (str_value) { > > + const char *tmp = str_value; > > + > > + if (g_str_has_prefix (tmp, NEW_TAG)) > > + tmp += strlen (NEW_TAG); > > + g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); > > + } > > > > this seems wrong. tmp is the "value", I think the key should be prefixed by > > NEW_TAG. > > The keys do not get prefixed by NEW_TAG at all, and none of the OLD_TAG keys > are exposed to clients. So while dhclient sends us options with > "new_ip_address" and "old_ip_address", clients have only ever seen > "ip_address". eg, the API of NMDHCP4Config/NMDHCP6Config is that they only > ever expose the *new* options. Thus the "new_" is redundant, and is stripped > off. > > The only reason to expose this stuff anyway is to allow scripts to handle DHCP > options that NM does not handle, like NIS servers and the NIS domain, or SIP > servers, or stuff like that. > > > also: > > > > + /* Filter out stuff that's not actually new DHCP options */ > > + for (p = ignored_options; *p; p++) { > > + if (!strcmp (*p, key)) > > + return; > > + } > > > > don't we care here about the NEW_TAG/OLD_TAG prefix? > > No, we don't. Only new options are exposed to clients, and the "new_" prefix > is stripped because it's redundant, as described above. > > Previously, the code kept all options (even the old ones) and only filtered > them/stripped the names when adding to the NMDHCPxConfig object. But since we > never use the old options, we might as well save some memory and just > immediately discard the options we never need or expose to clients. Which is > what this code is trying to do. Yes, so I think both my points are valid. all this should be: diff --git i/src/dhcp-manager/nm-dhcp-client.c w/src/dhcp-manager/nm-dhcp-client.c index 1746d04..11847b3 100644 --- i/src/dhcp-manager/nm-dhcp-client.c +++ w/src/dhcp-manager/nm-dhcp-client.c @@ -652,7 +652,11 @@ copy_option (const char * key, char *str_value = NULL; const char **p; static const char *ignored_options[] = { - "interface", "pid", "reason", "dhcp_message_type", NULL + "interface", + "pid", + "reason", + "dhcp_message_type", + NULL }; if (!G_VALUE_HOLDS (value, DBUS_TYPE_G_UCHAR_ARRAY)) { @@ -662,6 +666,10 @@ copy_option (const char * key, if (g_str_has_prefix (key, OLD_TAG)) return; + if (g_str_has_prefix (key, NEW_TAG)) + key += STRLEN (NEW_TAG); + if (!*key) + return; /* Filter out stuff that's not actually new DHCP options */ for (p = ignored_options; *p; p++) { @@ -670,15 +678,8 @@ copy_option (const char * key, } str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); - if (str_value) { - const char *tmp = str_value; - - if (g_str_has_prefix (tmp, NEW_TAG)) - tmp += strlen (NEW_TAG); - g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); - } - - g_free (str_value); + if (str_value) + g_hash_table_insert (hash, g_strdup (key), str_value); } void
ah, and with this change it should use g_hash_table_replace() instead of g_hash_table_insert(). Otherwise, if you have keys "X" and "NEW_X", you leak g_strdup(key). Actually, using g_hash_table_replace() in this case is always a safe bet.
(In reply to comment #5) > ah, and with this change it should use g_hash_table_replace() instead of > g_hash_table_insert(). > > Otherwise, if you have keys "X" and "NEW_X", you leak g_strdup(key). > Actually, using g_hash_table_replace() in this case is always a safe bet. Ah, g_hash_table_insert() destroys the new key, if such an old key already exists. So, replace and insert are both fine.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #1) > > > >> dhcp: filter DHCP options when setting them > > > + str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); > > > + if (str_value) { > > > + const char *tmp = str_value; > > > + > > > + if (g_str_has_prefix (tmp, NEW_TAG)) > > > + tmp += strlen (NEW_TAG); > > > + g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); > > > + } > > > > > > this seems wrong. tmp is the "value", I think the key should be prefixed by > > > NEW_TAG. > > > > The keys do not get prefixed by NEW_TAG at all, and none of the OLD_TAG keys > > are exposed to clients. So while dhclient sends us options with > > "new_ip_address" and "old_ip_address", clients have only ever seen > > "ip_address". eg, the API of NMDHCP4Config/NMDHCP6Config is that they only > > ever expose the *new* options. Thus the "new_" is redundant, and is stripped > > off. > > > > The only reason to expose this stuff anyway is to allow scripts to handle DHCP > > options that NM does not handle, like NIS servers and the NIS domain, or SIP > > servers, or stuff like that. > > > > > also: > > > > > > + /* Filter out stuff that's not actually new DHCP options */ > > > + for (p = ignored_options; *p; p++) { > > > + if (!strcmp (*p, key)) > > > + return; > > > + } > > > > > > don't we care here about the NEW_TAG/OLD_TAG prefix? > > > > No, we don't. Only new options are exposed to clients, and the "new_" prefix > > is stripped because it's redundant, as described above. > > > > Previously, the code kept all options (even the old ones) and only filtered > > them/stripped the names when adding to the NMDHCPxConfig object. But since we > > never use the old options, we might as well save some memory and just > > immediately discard the options we never need or expose to clients. Which is > > what this code is trying to do. > > > Yes, so I think both my points are valid. Yes, you are correct and your points are valid. > all this should be: > > diff --git i/src/dhcp-manager/nm-dhcp-client.c > w/src/dhcp-manager/nm-dhcp-client.c > index 1746d04..11847b3 100644 > --- i/src/dhcp-manager/nm-dhcp-client.c > +++ w/src/dhcp-manager/nm-dhcp-client.c > @@ -652,7 +652,11 @@ copy_option (const char * key, > char *str_value = NULL; > const char **p; > static const char *ignored_options[] = { > - "interface", "pid", "reason", "dhcp_message_type", NULL > + "interface", > + "pid", > + "reason", > + "dhcp_message_type", > + NULL These keys are the exact keys that dhclient will pass, so they will never be prefixed with "new_". Since they aren't actually DHCP options they won't have new/old prefix... > }; > > if (!G_VALUE_HOLDS (value, DBUS_TYPE_G_UCHAR_ARRAY)) { > @@ -662,6 +666,10 @@ copy_option (const char * key, > > if (g_str_has_prefix (key, OLD_TAG)) > return; > + if (g_str_has_prefix (key, NEW_TAG)) > + key += STRLEN (NEW_TAG); > + if (!*key) > + return; So I put this hunk below the ignored_options check. > /* Filter out stuff that's not actually new DHCP options */ > for (p = ignored_options; *p; p++) { > @@ -670,15 +678,8 @@ copy_option (const char * key, > } > > str_value = garray_to_string ((GArray *) g_value_get_boxed (value), key); > - if (str_value) { > - const char *tmp = str_value; > - > - if (g_str_has_prefix (tmp, NEW_TAG)) > - tmp += strlen (NEW_TAG); > - g_hash_table_insert (hash, g_strdup (key), g_strdup (tmp)); > - } > - > - g_free (str_value); > + if (str_value) > + g_hash_table_insert (hash, g_strdup (key), str_value); > } Done.
(In reply to comment #1) > >> dhcp: simplify DHCP states > > DHCP_STATE_DONE, /* END */ > - DHCP_STATE_FAIL /* failed or quit unexpectedly */ > + DHCP_STATE_FAIL, /* failed or quit unexpectedly */ > } NMDhcpState; > > possibly add > __DHCP_STATE_MAX, > DHCP_STATE_MAX = __DHCP_STATE_MAX - 1, > > and > static const char *state_table[DHCP_STATE_MAX + 1] = { Done. > >> core: consolidate DHCP client termination in dhcp[4|6]_fail() > > + dhcp6_cleanup (self, TRUE, FALSE); > has trailing white space Fixed. > >> dhcp: pass IP config in state signal > > indention in nm_dhcp_client_set_state() Fixed. > how about adding: > g_return_if_fail (!ip6_config || NM_IS_IP6_CONFIG (ip6_config)) > to dhcp6_state_changed? v4 respectively. Done. > >> dhcp: filter DHCP options when setting them > > + static const char *ignored_options[] = { > + "interface", "pid", "reason", "dhcp_message_type", NULL > + }; > > How about each option on a separate line, which is easier to read and results > in nicer diffs when adding options later. Done. > >> dhcp: consolidate more state logic in nm_dhcp_client_set_state() > > would be nice to split the commit in a trivial first, that just moves code > around. Done. > >> dhcp: pass IPv6 privacy preference to client class > > nm_dhcp_manager_start_ip4() > - timeout, dhcp_anycast_addr, hostname, FALSE, 0); > + timeout, dhcp_anycast_addr, hostname, FALSE, > NM_SETTING_IP6_CONFIG_PRIVACY_UNKNOWN); > > > > > finally: could you modify nm_dhcp_manager_start_ip6() not to take a NMSetting, > instead pass the needed parameters as is? I.e. privacy and hostname. > Same for v4. This decouples NMDHCPx from NMSetting. Done.
Looks good to me now. Just pushed one fixup! commit
> dhcp: handle DHCP clients that aren't subprocesses I don't love the out pid arg to ip4_start/ip6_start... If you made start_monitor() a public function, then the subclass could call it itself, and likewise could call nm_dhcp_client_stop_pid() from its stop() implementation, and then maybe the parent class wouldn't need to keep track of the pid at all? (There's one other place it's used, in a log message in nm_dhcp_client_stop(), which probably shouldn't be showing "DHCP client pid -1" in this case anyway.) This might not actually be an improvement. Just an idea. (Also, feel free to use pid_t rather than GPid if you prefer; GPid only exists to abstract between unix and windows.) > dhcp: simplify DHCP states >+ if (priv->state != DHCP_STATE_BOUND) { >+ nm_log_warn (priv->ipv6 ? LOGD_DHCP6 : LOGD_DHCP4, >+ "(%s): DHCPv%c client didn't bind to a lease.", extra level of indentation there >+ DHCP_STATE_UNKNOWN = 0, Prefix the values with "NM_"? > dhcp: pass DHCP options with BOUND state change At this point, couldn't NMDHCPClient just create the NMDHCP4Config/NMDHCP6Config object itself, and emit that, rather than emitting the hash table? > dhcp: consolidate more state logic in nm_dhcp_client_set_state() > @@ -612,7 +623,7 @@ copy_option (const char * key, All of these changes to copy_option could just be squashed into the previous rewrite couldn't they? ("dhcp: filter DHCP options when setting them")
(In reply to comment #10) > > dhcp: handle DHCP clients that aren't subprocesses > > I don't love the out pid arg to ip4_start/ip6_start... > > If you made start_monitor() a public function, then the subclass could call it > itself, and likewise could call nm_dhcp_client_stop_pid() from its stop() > implementation, and then maybe the parent class wouldn't need to keep track of > the pid at all? > > (There's one other place it's used, in a log message in nm_dhcp_client_stop(), > which probably shouldn't be showing "DHCP client pid -1" in this case anyway.) I like it. Done. > (Also, feel free to use pid_t rather than GPid if you prefer; GPid only exists > to abstract between unix and windows.) Also done. > > dhcp: simplify DHCP states > > >+ if (priv->state != DHCP_STATE_BOUND) { > >+ nm_log_warn (priv->ipv6 ? LOGD_DHCP6 : LOGD_DHCP4, > >+ "(%s): DHCPv%c client didn't bind to a lease.", > > extra level of indentation there Fixed. > >+ DHCP_STATE_UNKNOWN = 0, > > Prefix the values with "NM_"? Done. > > dhcp: pass DHCP options with BOUND state change > > At this point, couldn't NMDHCPClient just create the > NMDHCP4Config/NMDHCP6Config object itself, and emit that, rather than emitting > the hash table? Yes, but we don't change the DHCP config objects over the lifetime of the NMDevice, so doing this would be a bit less efficient for clients since they'd have to tear down their old DHCP4/6Config and then create a new one and read the properties, instead of just replacing the properties like they can now. That's well within the scope of our dbus interface though, so creating a new DHCPConfig with each lease change wouldn't be a backwards compat problem. I'm happy to do what you prefer. > > dhcp: consolidate more state logic in nm_dhcp_client_set_state() > > > @@ -612,7 +623,7 @@ copy_option (const char * key, > > All of these changes to copy_option could just be squashed into the previous > rewrite couldn't they? ("dhcp: filter DHCP options when setting them") Yeah, I have no idea how those ended up there. Mis-sqaush I think. Fixed! Back up for review.
Did a quick re-review... LGTM. Pushed minor fixup! commit
(In reply to comment #11) > > > dhcp: pass DHCP options with BOUND state change > > > > At this point, couldn't NMDHCPClient just create the > > NMDHCP4Config/NMDHCP6Config object itself, and emit that, rather than emitting > > the hash table? > > Yes, but we don't change the DHCP config objects over the lifetime of the > NMDevice, so doing this would be a bit less efficient for clients since they'd > have to tear down their old DHCP4/6Config and then create a new one and read > the properties, instead of just replacing the properties like they can now. To clarify, I meant to write "over the lifetime of the activation." So clients do have to deal with a changing DHCPConfig any time the device acitvates/deactivates.
(In reply to comment #11) > > > dhcp: handle DHCP clients that aren't subprocesses > > > > I don't love the out pid arg to ip4_start/ip6_start... > > > > If you made start_monitor() a public function, then the subclass could call it > > itself, and likewise could call nm_dhcp_client_stop_pid() from its stop() > > implementation, and then maybe the parent class wouldn't need to keep track of > > the pid at all? > I like it. Done. Nice. (I was imagining that NMDHCPClient would lose priv->pid, and the subclasses would keep track of their pid themselves, but maybe that doesn't work as well for whatever reason.) > > (There's one other place it's used, in a log message in nm_dhcp_client_stop(), > > which probably shouldn't be showing "DHCP client pid -1" in this case anyway.) This is still an issue, except now it will *always* print "pid -1" because priv->pid gets unset before that point now. > > > dhcp: pass DHCP options with BOUND state change > > > > At this point, couldn't NMDHCPClient just create the > > NMDHCP4Config/NMDHCP6Config object itself, and emit that, rather than emitting > > the hash table? ... > I'm happy to do what you prefer. It sounds like there are good reasons to keep it as it is.
(In reply to comment #14) > > > (There's one other place it's used, in a log message in nm_dhcp_client_stop(), > > > which probably shouldn't be showing "DHCP client pid -1" in this case anyway.) > > This is still an issue, except now it will *always* print "pid -1" because > priv->pid gets unset before that point now. Fixed; saved priv->pid to a temp variable and use it in the log statement.
looks good to me now
Merged to master after ack from thaller too (via IRC).
[FTR] merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=afebbb6ef8db81270beb8d55fa03000890ca8875