Struggling with more inconsistant uses/assumptions around the global
time stamp (cur_time) I finally concluded it was fatally flawed. Or
at least more trouble than it is worth.
So,
1) Use time(NULL) to get current time where required,
2) Add local variables cur_time where the time is used mulitple
times in a single function,
3) Add set_timer_interval() so an accurate timeout is set from a
given interval.
Most obviously to me, this makes several machines that think a few
seconds to get link, or whose cards can't report link, work with
far few packet exchanges. Previously the cur_time was set before
the link state test. So timeouts of less than a few seconds always
triggered immediately causing multiple DHCPDISCOVER or REQUEST
packets to be sent immediately.
With this diff I consistantly get the minimal DISCOVER/OFFER/REQUEST/ACK
exchange.
Thoughts? Testing in intriguing environments?
.... Ken
Index: clparse.c
===================================================================
RCS file: /cvs/src/sbin/dhclient/clparse.c,v
retrieving revision 1.39
diff -u -p -r1.39 clparse.c
--- clparse.c 22 Aug 2012 00:14:42 -0000 1.39
+++ clparse.c 26 Aug 2012 03:53:20 -0000
@@ -471,7 +471,7 @@ parse_client_lease_statement(FILE *cfile
* active.
*/
if (client->active) {
- if (client->active->expiry < cur_time)
+ if (client->active->expiry < time(NULL))
free_client_lease(client->active);
else if (addr_eq(client->active->address, lease->address))
free_client_lease(client->active);
Index: dhclient.c
===================================================================
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.151
diff -u -p -r1.151 dhclient.c
--- dhclient.c 22 Aug 2012 00:14:42 -0000 1.151
+++ dhclient.c 26 Aug 2012 03:53:21 -0000
@@ -66,8 +66,6 @@
#define DEFAULT_LEASE_TIME 43200 /* 12 hours... */
#define TIME_MAX 2147483647
-time_t cur_time;
-
char *path_dhclient_conf = _PATH_DHCLIENT_CONF;
char *path_dhclient_db = NULL;
@@ -220,7 +218,7 @@ routehandler(void)
ifam->ifam_addrs) != AF_INET)
break;
/* XXX check addrs like RTM_NEWADDR instead of this? */
- if (scripttime == 0 || cur_time < scripttime + 10)
+ if (scripttime == 0 || time(NULL) < scripttime + 10)
break;
errmsg = "interface address deleted";
goto die;
@@ -325,7 +323,6 @@ main(int argc, char *argv[])
log_perror = 0;
tzset();
- time(&cur_time);
memset(&sockaddr_broadcast, 0, sizeof(sockaddr_broadcast));
sockaddr_broadcast.sin_family = AF_INET;
@@ -487,7 +484,7 @@ state_reboot(void)
flags. */
make_request(client->active);
client->destination = iaddr_broadcast;
- client->first_sending = cur_time;
+ client->first_sending = time(NULL);
client->interval = 0;
/* Send out the first DHCPREQUEST packet. */
@@ -507,7 +504,7 @@ state_init(void)
client->xid = client->packet.xid;
client->destination = iaddr_broadcast;
client->state = S_SELECTING;
- client->first_sending = cur_time;
+ client->first_sending = time(NULL);
client->interval = 0;
/* Add an immediate timeout to cause the first DHCPDISCOVER packet
@@ -523,6 +520,7 @@ void
state_selecting(void)
{
struct client_lease *lp, *next, *picked;
+ time_t cur_time;
/* Cancel state_selecting and send_discover timeouts, since either
one could have got us here. */
@@ -554,6 +552,8 @@ state_selecting(void)
picked->next = NULL;
+ time(&cur_time);
+
/* If it was a BOOTREPLY, we can just take the address right now. */
if (!picked->options[DHO_DHCP_MESSAGE_TYPE].len) {
client->new = picked;
@@ -592,6 +592,7 @@ void
dhcpack(struct iaddr client_addr, struct option_data *options)
{
struct client_lease *lease;
+ time_t cur_time;
if (client->state != S_REBOOTING &&
client->state != S_REQUESTING &&
@@ -640,6 +641,8 @@ dhcpack(struct iaddr client_addr, struct
client->new->rebind = client->new->renewal +
client->new->renewal / 2 + client->new->renewal / 4;
+ time(&cur_time);
+
client->new->expiry += cur_time;
/* Lease lengths can never be negative. */
if (client->new->expiry < cur_time)
@@ -680,7 +683,7 @@ bind_lease(void)
note("bound to %s -- renewal in %lld seconds.",
piaddr(client->active->address),
- (long long)(client->active->renewal - cur_time));
+ (long long)(client->active->renewal - time(NULL)));
client->state = S_BOUND;
reinitialize_interface();
go_daemon();
@@ -707,7 +710,7 @@ state_bound(void)
} else
client->destination = iaddr_broadcast;
- client->first_sending = cur_time;
+ client->first_sending = time(NULL);
client->interval = 0;
client->state = S_RENEWING;
@@ -784,7 +787,7 @@ dhcpoffer(struct iaddr client_addr, stru
/* If the selecting interval has expired, go immediately to
state_selecting(). Otherwise, time out into
state_selecting at the select interval. */
- if (stop_selecting <= cur_time)
+ if (stop_selecting <= time(NULL))
state_selecting();
else {
set_timeout(stop_selecting, state_selecting);
@@ -897,6 +900,7 @@ dhcpnak(struct iaddr client_addr, struct
void
send_discover(void)
{
+ time_t cur_time;
int interval, increase = 1;
/* Figure out how long it's been since we started transmitting. */
@@ -953,7 +957,7 @@ send_discover(void)
/* Send out a packet. */
send_packet(inaddr_any, &sockaddr_broadcast, NULL);
- set_timeout(cur_time + client->interval, send_discover);
+ set_timeout_interval(client->interval, send_discover);
}
/*
@@ -967,6 +971,7 @@ state_panic(void)
{
struct client_lease *loop = client->active;
struct client_lease *lp;
+ time_t cur_time;
note("No DHCPOFFERS received.");
@@ -976,6 +981,7 @@ state_panic(void)
goto activate_next;
/* Run through the list of leases and see if one can be used. */
+ time(&cur_time);
while (client->active) {
if (client->active->expiry > cur_time) {
note("Trying recorded lease %s",
@@ -989,8 +995,7 @@ state_panic(void)
yet need renewal, go into BOUND state and
timeout at the renewal time. */
if (!script_go()) {
- if (cur_time <
- client->active->renewal) {
+ if (cur_time < client->active->renewal) {
client->state = S_BOUND;
note("bound: renewal in %lld seconds.",
(long long)(client->active->renewal
@@ -1042,7 +1047,7 @@ activate_next:
script_init("FAIL");
script_go();
client->state = S_INIT;
- set_timeout(cur_time + config->retry_interval, state_init);
+ set_timeout_interval(config->retry_interval, state_init);
go_daemon();
}
@@ -1051,10 +1056,13 @@ send_request(void)
{
struct sockaddr_in destination;
struct in_addr from;
+ time_t cur_time;
int interval;
+ time(&cur_time);
+
/* Figure out how long it's been since we started transmitting. */
- interval = cur_time - client->first_sending;
+ interval = (int)(cur_time - client->first_sending);
/* If we're in the INIT-REBOOT or REQUESTING state and we're
past the reboot timeout, go to INIT and see if we can
@@ -1142,7 +1150,7 @@ send_request(void)
/* Send out a packet. */
send_packet(from, &destination, NULL);
- set_timeout(cur_time + client->interval, send_request);
+ set_timeout_interval(client->interval, send_request);
}
void
Index: dhcpd.h
===================================================================
RCS file: /cvs/src/sbin/dhclient/dhcpd.h,v
retrieving revision 1.78
diff -u -p -r1.78 dhcpd.h
--- dhcpd.h 22 Aug 2012 00:14:42 -0000 1.78
+++ dhcpd.h 26 Aug 2012 03:53:21 -0000
@@ -252,6 +252,7 @@ void reinitialize_interface(void);
void dispatch(void);
void got_one(void);
void set_timeout(time_t, void (*)(void));
+void set_timeout_interval(time_t, void (*)(void));
void cancel_timeout(void);
int interface_status(char *);
int interface_link_forceup(char *);
@@ -279,7 +280,6 @@ char *piaddr(struct iaddr);
/* dhclient.c */
extern char *path_dhclient_conf;
extern char *path_dhclient_db;
-extern time_t cur_time;
extern int log_perror;
extern int routefd;
Index: dispatch.c
===================================================================
RCS file: /cvs/src/sbin/dhclient/dispatch.c,v
retrieving revision 1.54
diff -u -p -r1.54 dispatch.c
--- dispatch.c 18 Aug 2012 00:20:01 -0000 1.54
+++ dispatch.c 26 Aug 2012 03:53:21 -0000
@@ -122,7 +122,7 @@ dispatch(void)
{
int count, to_msec;
struct pollfd fds[2];
- time_t howlong;
+ time_t cur_time, howlong;
void (*func)(void);
do {
@@ -174,9 +174,6 @@ another:
/* Wait for a packet or a timeout... XXX */
count = poll(fds, 2, to_msec);
- /* Time may have moved on while we polled! */
- time(&cur_time);
-
/* Not likely to be transitory... */
if (count == -1) {
if (errno == EAGAIN || errno == EINTR) {
@@ -319,6 +316,13 @@ void
set_timeout(time_t when, void (*where)(void))
{
timeout.when = when;
+ timeout.func = where;
+}
+
+void
+set_timeout_interval(time_t secs, void (*where)(void))
+{
+ timeout.when = time(NULL) + secs;
timeout.func = where;
}
