[PATCH] udhcpd: account for script delay in lease
John Schroeder
jschroed at gmail.com
Mon Dec 22 05:57:15 UTC 2014
That solution is much better and simpler. It nicely covers the typical
case of a short delay caused by scripts.
The thing wrong with immediately going to the renew state is that the
renew state could continue after the lease expired. (Possibly only a
late renew request but the unlikely worst-case scenario being a renew
attempt after the lease expired.)
It might be superfluous but how about going to the discover state (init
selecting) if the timeout (half the lease time) has been exceeded? This
correctly handles the case when the lease has expired. The very rare
case of being in the last half of the lease is also dealt with cleanly
(rather than continuing beyond the lease expiration).
Below is a patch with a check if the timeout is exceeded. I am not
certain if it is worth the trade off of having the extra code but it
does handle these cases. Again, this case is very unlikely so it might
not be worth having this code since the current code (plus script
duration) would work most of the time.
Signed-off-by: John Schroeder <jschroed at gmail.com>
--- a/networking/udhcp/dhcpc.c
+++ b/networking/udhcp/dhcpc.c
@@ -1742,7 +1742,14 @@
bb_info_msg("Lease of %s obtained, lease time %u",
inet_ntoa(temp_addr), (unsigned)lease_seconds);
requested_ip = packet.yiaddr;
+
+ timestamp_before_wait = monotonic_sec();
udhcp_run_script(&packet, state == REQUESTING ?
"bound" : "renew");
+ already_waited_sec = (unsigned)monotonic_sec() -
timestamp_before_wait;
+
+ if (already_waited_sec > timeout) { /* immediately go
to discover */
+ timeout = 0;
+ }
state = BOUND;
change_listen_mode(LISTEN_NONE);
@@ -1760,7 +1767,6 @@
#endif
/* make future renew packets use different xid */
/* xid = random_xid(); ...but why bother? */
- already_waited_sec = 0;
continue; /* back to main loop */
}
if (*message == DHCPNAK) {
> On Mon, Dec 1, 2014 at 4:45 AM, John Schroeder<jschroed at gmail.com> wrote:
>> Hello all,
>>
>> I ran into an issue where I was seeing a long delay in the scripts called in
>> udhcp_run_script. I was using an old version of OpenWrt (kamikaze) and a
>> satellite modem. An NTP script was being called and the modem would
>> sometimes take a long time to respond to the DNS lookup when it was offline.
> What would happen if script never returns?
>
>> This delay started affecting my lease time. The lease that I would get from
>> my satellite modem before it was online would be short: only 60 seconds. The
>> delay with NTP and the modem would typically be about 18 seconds. This would
>> cause the first DHCP renew request from dhcpc to be a little late. Under
>> certain circumstances, I could even see the first DHCP renew to occur after
>> the lease had expired!
>>
>> I patched dhcpc.c in my build using the following patch below. I used the
>> existing variable already_waited_sec to calculate the number of seconds it
>> took to complete running the scripts. If the delay was not greater than the
>> timeout (i.e. half the lease time), then the code would simply send the
>> renew using the already_waited_sec (e.g. 30 sec - 18 sec = 12 sec to send
>> the renew). If the delay was greater than the timeout but less than the
>> lease time, the code subtracts the time out from the already_waited_sec,
>> halves the time out, and continues as before. (E.g. already_waited_sec is 40
>> sec, then the timeout would be 15 sec and already_waited_sec would be
>> changed to 10 sec. This would cause the renew to be sent out 5 seconds later
>> at the 45 second mark.) This happens in a while loop while the timeout is
>> greater than 0. (Note that if a renew event is missed, it is simply skipped.
>> E.g. In a 60 sec lease, the 30 sec mark could be skipped and the next renew
>> sent at 45 seconds.) If the already_waited_sec ends up being greater than
>> the timeout, the code will not send a renew and re-enter the requesting
>> state.
> The above is complicated. Why do you want to do that?
>
>> /* enter bound state */
>> timeout = lease_seconds / 2;
>> + timestamp_before_wait = (unsigned)monotonic_sec();
>> temp_addr.s_addr = packet.yiaddr;
>> bb_info_msg("Lease of %s obtained, lease time %u",
>> inet_ntoa(temp_addr), (unsigned)lease_seconds);
>> @@ -1774,7 +1775,21 @@
>> #endif
>> /* make future renew packets use different xid */
>> /* xid = random_xid(); ...but why bother? */
>> - already_waited_sec = 0;
>> + already_waited_sec = (unsigned)monotonic_sec() -
>> timestamp_before_wait;
>> + /* If already beyond timeout, skip the next renew
>> + * event. Continue skipping renews and halving the
>> + * timeout until the next timeout is less that the
>> + * already waited time or the timeout is 0.
>> + */
>> + while ((already_waited_sec > timeout) && (timeout > 0)) {
>> + already_waited_sec -= timeout;
>> + timeout >>= 1;
>> + }
>> + /* If timeout is 0, then the lease has expired. Set
>> + * already_waited_sec to 0.
>> + */
>> + if (timeout == 0)
>> + already_waited_sec = 0;
>> continue; /* back to main loop */
>> }
>> if (*message == DHCPNAK) {
> How about a straightforward code which sets
> already_waited_sec to script duration? -
>
>
> @@ -1756,7 +1757,10 @@ int udhcpc_main(int argc UNUSED_PARAM, c
> bb_info_msg("Lease of %s obtained,
> lease time %u",
> inet_ntoa(temp_addr),
> (unsigned)lease_seconds);
> requested_ip = packet.yiaddr;
> +
> + start = monotonic_sec();
> udhcp_run_script(&packet, state ==
> REQUESTING ? "bound" : "renew");
> + already_waited_sec =
> (unsigned)monotonic_sec() - start;
>
> state = BOUND;
> change_listen_mode(LISTEN_NONE);
> @@ -1774,7 +1778,7 @@ int udhcpc_main(int argc UNUSED_PARAM, c
> #endif
> /* make future renew packets use
> different xid */
> /* xid = random_xid(); ...but why bother? */
> - already_waited_sec = 0;
>
>
> If already_waited_sec > timeout, then existing code will immediately
> go into renew (or rebind) state and start sending packets immediately.
> What's wrong with that?
More information about the busybox
mailing list