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 <[email protected]>
--- 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<[email protected]> 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?
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
