GNOME Bugzilla – Bug 740277
internal dhcp fixes [danw/dhcp-internal-bgo740277]
Last modified: 2014-11-24 15:54:06 UTC
NetworkManager.conf.5 needs to document dhcp=internal First, the fact that it even exists. (It currently only mentions dhclient and dhcpcd.) Second, its limitations. At the moment, the two I know of are (a) it doesn't support DHCPv6, and (b) it discards all options it doesn't recognize, which is most of them. In particular, "domain_search", "nis_domain", "nis_servers", and "netbios_name_servers" will not even get propagated to the NMIP4Config.
I'm going to repurpose this into a generic dhcp=internal bug Pushed fixes to danw/dhcp-internal-bgo740277, including a rebase of the systemd-dhcp code. As compared to Thomas's earlier th/systemd-dhcp, the big difference in my rebase is that I #ifdeffed out the getrandom() code, because it depends on super-new kernel/headers anyway.
Looks good to me.
> dhcp: update systemd DHCP code The import is somehow different from 1dc24d5f48b384c48d5182964579758d7dcbdce2 commit. > dhcp: re-fix system-dhcp code after re-import > dhcp: update nm-dhcp-systemd code for system-dhcp re-import The fist commit does not compile. Could you mention that in the commit message? Of just squash both together? > dhcp-manager: Simplify nm-dhcp-systemd lease-handling code + time_t end_time; + end_time = MIN ((guint64) time (NULL) + lifetime, G_MAXUINT32 - 1); + LOG_LEASE (LOGD_DHCP4, " expires %s", ctime (&end_time)); add_option_u32 (options, + (guint32) end_time); the glib MIN() macro evaluates arguments more the once. And I would not cast to guint64. gint64 is far large enough too. But anyway, how about adding a add_option_s64() function instead? Rest LGTM
I pushed one commit on your branch: >> dhcp: pass device specific route metric to ...
Also upstream systemd just merged all my outstanding patches...
Er... oops, pushed without addressing Thomas's comments (In reply to comment #3) > > dhcp: update systemd DHCP code > > The import is somehow different from 1dc24d5f48b384c48d5182964579758d7dcbdce2 > commit. I may have copied in the wrong commit ID? At any rate, it's been rebased to a newer commit now anyway > > dhcp: re-fix system-dhcp code after re-import > > dhcp: update nm-dhcp-systemd code for system-dhcp re-import > > The fist commit does not compile. Could you mention that in the commit message? > Of just squash both together? I thought it was better to distinguish the changes to their code (to make it usable) vs changes to our code (to update for API changes), so I left it as two commits. (And didn't change the commit message, because, as above, I forgot about your comment until after pushing.) > > dhcp-manager: Simplify nm-dhcp-systemd lease-handling code > > + time_t end_time; > > + end_time = MIN ((guint64) time (NULL) + lifetime, G_MAXUINT32 - 1); > + LOG_LEASE (LOGD_DHCP4, " expires %s", ctime (&end_time)); > add_option_u32 (options, > + (guint32) end_time); > > the glib MIN() macro evaluates arguments more the once. Yes, I originally tried to avoid that, but... oh, hm. In an earlier version it would have required adding another temporary variable, but I guess at this point I could just have done "end_time = time (NULL); end_time = MIN (...". But it's not really a huge deal; calling time() twice is still going to be infinitely faster than getting a response from the DHCP server. > And I would not cast to guint64. gint64 is far large enough too. The value here can never meaningfully be negative, so using an unsigned type made more sense to me. Although, sigh, the code is wrong anyway... time_t is signed, so on x86 if the lease extends past Y2.038k we'll end up logging an expiration date in the past (though still keeping the correct value internally). > But anyway, how about adding a add_option_s64() function instead? Hm... I guess since it's a string we can make it as big as we want (since it's not actually going to start exceeding MAXUINT32 for a long time anyway). Though again I see no reason to use a signed type. (In reply to comment #4) > I pushed one commit on your branch: > > >> dhcp: pass device specific route metric to ... which I also failed to pull in. I think it should just be calling nm_dhcp_client_get_priority() like the other call to lease_to_ip4_config() does?
(In reply to comment #6) > (In reply to comment #4) > > I pushed one commit on your branch: > > > > >> dhcp: pass device specific route metric to ... > > which I also failed to pull in. > > I think it should just be calling nm_dhcp_client_get_priority() like the other > call to lease_to_ip4_config() does? Yes.
pushed danw/dhcp-internal-fixup-bgo740277 with a fix for the end_time messiness, plus Thomas's route metric fix (In reply to comment #7) > > I think it should just be calling nm_dhcp_client_get_priority() like the other > > call to lease_to_ip4_config() does? > > Yes. Actually, no; it's called before an NMDHCPClient is created, so it can't call that. So we have to pass it in from NMDevice, as Thomas's patch does.
Fixups look good to me.
end_time = (guint64) time (NULL) + lifetime; - LOG_LEASE (LOGD_DHCP4, " expires in %" G_GUINT64_FORMAT " seconds", end_time); + LOG_LEASE (LOGD_DHCP4, " expires in %" G_GUINT64_FORMAT " seconds", lifetime); Rest LGTM
pushed 13d9b28 dhcp: pass device specific route metric to nm_dhcp_systemd_get_lease_ip_configs() 9013fd4 dhcp: fix expiry time logging/exporting