dhclient: Set default ARP resolution wait to 250 ms, with an option to make it 0 ms #1368

ica574 · 2024-08-06T13:52:43Z

Make the default ARP resolution wait time 250 ms, and add an option within dhclient to remove it altogether.

cperciva · 2024-08-06T16:18:52Z

@ica574 Looks like the style checker wants "static const" rather than "const static". Can you change that?

It's also complaining about the timespeccmp calls but that's a bug in the style checker; please leave those as they are.

@bsdimp Do we ask for signed-off-by lines in github PRs? I'm sure Isaac can add them, I just hadn't noticed them before and wasn't sure if this was something you(?) put into the style checker deliberately.

ica574 · 2024-08-06T16:27:09Z

@ica574 Looks like the style checker wants "static const" rather than "const static". Can you change that?

It's also complaining about the timespeccmp calls but that's a bug in the style checker; please leave those as they are.

@bsdimp Do we ask for signed-off-by lines in github PRs? I'm sure Isaac can add them, I just hadn't noticed them before and wasn't sure if this was something you(?) put into the style checker deliberately.

@cperciva Done! I've changed "const static" to "static const".

brooksdavis · 2024-08-06T17:37:38Z

sbin/dhclient/dhclient.c

@@ -120,6 +121,8 @@ struct pidfh *pidfile;
 */
 #define TIME_MAX        ((((time_t) 1 << (sizeof(time_t) * CHAR_BIT - 2)) - 1) * 2 + 1)

+static struct timespec arp_timeout = { .tv_sec = 2, .tv_nsec = 0 };
+const struct timespec zero_timespec = { .tv_sec = 0, .tv_nsec = 0 };


It seems like this could be static?

brooksdavis · 2024-08-06T17:43:48Z

sbin/dhclient/dhcpd.h

 extern time_t cur_time;
 extern int log_priority;
 extern int log_perror;

+extern const struct timespec zero_timespec;


Why declare this when it's only used in dhclient.c?

cperciva · 2024-08-06T18:45:43Z

Oops, I told @ica574 to make zero_timespec extern since I thought it was being used in multiple files. I think I forgot how much code was all in dhclient.c.

Isaac, please mark it static and remove it from dhcpd.h

concussious

Everyone doesn't always do it, but it'd be nice if the commit message explains why you want to do that for someone unfamiliar with the topic.

concussious · 2024-08-06T20:32:34Z

sbin/dhclient/dhclient.8

@@ -95,6 +95,9 @@ Forces
 .Nm
 to reject leases with unknown options in them.
 The default behaviour is to accept such lease offers.
+.It Fl n
+Don't wait for ARP resolution within
+.Nm


Please keep the flags alphabetized by moving the added text to after line 84. Also the sentances explaining other flags are ending with a period. Thanks!

concussious

My wheelhouse is only manuals, but manual LGTM! Congrats on your project, changing the world is so exciting!

tomhukins · 2024-08-06T21:20:58Z

libexec/rc/rc.conf

@@ -138,6 +138,7 @@ dhclient_flags=""		# Extra flags to pass to dhcp client.
 #dhclient_flags_em0=""		# Extra dhclient flags for em0 only
 background_dhclient="NO"	# Start dhcp client in the background.
 #background_dhclient_em0="YES"	# Start dhcp client on em0 in the background.
+dhclient_noarpwait="NO"	# Trust IP address from DHCP and don't ARP


The double negative in dhclient_noarpwait="NO" reads awkwardly. Reversing its logic, naming it dhclient_wait_for_arp, and setting its default value to "YES" might make it easier to understand.

Good point. I suggested "noarpwait" since that's what the -n flag means (and it needs to be that way around to keep the previous "no flags means wait for ARP" behaviour) but you're quite right that for a new rc.conf setting we can reverse that.

@ica574 How about dhclient_arpwait="YES" # ARP IP addresses before using them and a corresponding change in the rc.d script?

Sounds good. I'll implement it!

cperciva · 2024-08-07T15:40:39Z

libexec/rc/rc.conf

@@ -138,6 +138,7 @@ dhclient_flags=""		# Extra flags to pass to dhcp client.
 #dhclient_flags_em0=""		# Extra dhclient flags for em0 only
 background_dhclient="NO"	# Start dhcp client in the background.
 #background_dhclient_em0="YES"	# Start dhcp client on em0 in the background.
+dhclient_arpwait="YES"	# Trust IP address from DHCP and don't ARP


You forgot to update the comment

My bad, should be fixed now.

cperciva · 2024-08-07T15:41:45Z

libexec/rc/rc.d/dhclient

@@ -48,6 +48,10 @@ dhclient_prestart()
 		rc_flags="${rc_flags} -b"
 	fi

+	dhclient_arpwait=$(get_if_var $ifn dhclient_arpwait_IF $dhclient_arpwait)
+	if checkyesno dhclient_arpwait; then


and this is now backwards; we need to add -n if dhclient_arpwait is set to no.

concussious · 2024-08-07T16:24:10Z

Should we add an entry for dhclient_arpwait to rc.conf.5?

cperciva · 2024-08-07T16:51:39Z

libexec/rc/rc.conf

@@ -138,6 +138,7 @@ dhclient_flags=""		# Extra flags to pass to dhcp client.
 #dhclient_flags_em0=""		# Extra dhclient flags for em0 only
 background_dhclient="NO"	# Start dhcp client in the background.
 #background_dhclient_em0="YES"	# Start dhcp client on em0 in the background.
+dhclient_arpwait="YES"	# Wait for ARP resolution


Looks like we need an extra tab to line the comment up with the rest? Or is that just github being wonky?

cperciva · 2024-08-07T16:54:32Z

Should we add an entry for dhclient_arpwait to rc.conf.5?

Yes, that's probably a good idea. @ica574 Can you add this? Write something about not ARPing IP addresses before using them and this making the system boot faster on networks where the DHCP server is guaranteed to know whether an address is available.

cperciva · 2024-08-07T17:06:24Z

@ica574 Can you add Signed-off-by tags to all of the commits? I think git rebase --signoff might do this.

Introduce a new function, add_timeout_timespec(), to use timespec structs to handle timeouts. Make add_timeout() into a wrapper for the latter function to retain compatibility with the rest of the codebase. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

cperciva · 2024-08-08T15:35:13Z

share/man/man5/rc.conf.5

+.It Va dhclient_arpwait
+.Pq Vt bool
+Set to
+.Dq Li YES


YES is the default, so for rc.conf(5) we should say what happens if we set it to NO.

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd#1368

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd#1368

Introduce a new function, add_timeout_timespec(), to use timespec structs to handle timeouts. Make add_timeout() into a wrapper for the latter function to retain compatibility with the rest of the codebase. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 16a235f)

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 76e0ffd)

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit f0a3897)

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit b51569a)

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 84fce4b)

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 503adcd)

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit e4482bf)

Introduce a new function, add_timeout_timespec(), to use timespec structs to handle timeouts. Make add_timeout() into a wrapper for the latter function to retain compatibility with the rest of the codebase. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 16a235f)

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 76e0ffd)

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit f0a3897)

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit b51569a)

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 84fce4b)

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit 503adcd)

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: #1368 (cherry picked from commit e4482bf)

Introduce a new function, add_timeout_timespec(), to use timespec structs to handle timeouts. Make add_timeout() into a wrapper for the latter function to retain compatibility with the rest of the codebase. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Introduce a new function, add_timeout_timespec(), to use timespec structs to handle timeouts. Make add_timeout() into a wrapper for the latter function to retain compatibility with the rest of the codebase. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Introduce a new rc.conf option to not wait for ARP resolution within dhclient. This is plausible on many modern networks where it is possible to trust the DHCP server to know whether an IP address is available. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org> MFC after: 10 days Reviwed by: cperciva, brooks, Tom Hukins, Alexander Ziaee Pull Request: freebsd/freebsd-src#1368

cperciva requested review from bsdimp and cperciva August 6, 2024 16:14

ica574 force-pushed the dhclient-patch branch from bb219c1 to 32fd883 Compare August 6, 2024 16:24

brooksdavis reviewed Aug 6, 2024

View reviewed changes

concussious suggested changes Aug 6, 2024

View reviewed changes

ica574 force-pushed the dhclient-patch branch from 32fd883 to f0b74b4 Compare August 6, 2024 20:50

concussious approved these changes Aug 6, 2024

View reviewed changes

tomhukins reviewed Aug 6, 2024

View reviewed changes

ica574 force-pushed the dhclient-patch branch from f0b74b4 to 225a845 Compare August 7, 2024 14:03

cperciva reviewed Aug 7, 2024

View reviewed changes

ica574 force-pushed the dhclient-patch branch from 225a845 to a05565c Compare August 7, 2024 15:53

cperciva reviewed Aug 7, 2024

View reviewed changes

ica574 force-pushed the dhclient-patch branch 2 times, most recently from 94f1a9f to 89b16d8 Compare August 8, 2024 06:43

ica574 added 6 commits August 8, 2024 08:45

dhclient: Timeouts for entering state_selecting

aa7902b

Use the new add_timeout_timespec() API to handle timeouts for state_selecting within dhclient.c. No functional change intended. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

dhclient: Use clock_gettime() instead of time()

caeab8f

Change the use of time() to clock_gettime() to have millisecond-accurate rather than second-accurate timeouts. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

dhclient: Make arp_timeout configurable

11876b4

Make arp_timeout available to dhclient.c, set the default timeout to 250 ms, and provide a new command-line argument, 'n' for setting the timeout to 0. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

dhclient: Update dhclient man page for n flag

8713ace

Document new n flag for disabling ARP resolution within dhclient. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

ica574 force-pushed the dhclient-patch branch from 89b16d8 to 4cb0742 Compare August 8, 2024 06:46

cperciva reviewed Aug 8, 2024

View reviewed changes

dhclient: Update rc.conf.5 with dhclient_arpwait

c22b6c8

Add new dhclient_arpwait option to rc.conf.5, with information about what it does, and cases in which it could be disabled. Sponsored by: Google LLC (GSoC 2024) Signed-off-by: Isaac Cilia Attard <icattard@FreeBSD.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dhclient: Set default ARP resolution wait to 250 ms, with an option to make it 0 ms #1368

dhclient: Set default ARP resolution wait to 250 ms, with an option to make it 0 ms #1368

ica574 commented Aug 6, 2024

cperciva commented Aug 6, 2024

ica574 commented Aug 6, 2024

brooksdavis Aug 6, 2024

brooksdavis Aug 6, 2024

cperciva commented Aug 6, 2024

concussious left a comment

concussious Aug 6, 2024

concussious left a comment

tomhukins Aug 6, 2024

cperciva Aug 6, 2024

ica574 Aug 7, 2024

cperciva Aug 7, 2024

ica574 Aug 7, 2024

cperciva Aug 7, 2024

concussious commented Aug 7, 2024

cperciva Aug 7, 2024

cperciva commented Aug 7, 2024

cperciva commented Aug 7, 2024

cperciva Aug 8, 2024

dhclient: Set default ARP resolution wait to 250 ms, with an option to make it 0 ms #1368

dhclient: Set default ARP resolution wait to 250 ms, with an option to make it 0 ms #1368

Conversation

ica574 commented Aug 6, 2024

cperciva commented Aug 6, 2024

ica574 commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cperciva commented Aug 6, 2024

concussious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concussious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concussious commented Aug 7, 2024

Choose a reason for hiding this comment

cperciva commented Aug 7, 2024

cperciva commented Aug 7, 2024

Choose a reason for hiding this comment