Hi,
On Mon, 2014-02-10 at 16:29 -0800, Andrew LeCain wrote:
> The existing renew/rebind mechanism doesn't correctly follow the
> procedure as indicated in rfc2131 section 4.4.5 dictating the
> resending of renewal and rebinding packets. This change sets up
> three seperate timers for T1, T2, and expiration and correctly
> sends retries for renew and rebind with a logarithmic backoff.
> ---
> gdhcp/client.c | 205
> +++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 123 insertions(+), 82 deletions(-)
>
> diff --git a/gdhcp/client.c b/gdhcp/client.c
> index b24d19d..3b4ec9c 100644
> --- a/gdhcp/client.c
> +++ b/gdhcp/client.c
> @@ -100,6 +100,9 @@ struct _GDHCPClient {
> uint8_t ack_retry_times;
> uint8_t conflicts;
> guint timeout;
> + guint t1_timeout;
> + guint t2_timeout;
> + guint lease_timeout;
> guint listener_watch;
> GIOChannel *listener_channel;
> GList *require_list;
> @@ -271,6 +274,26 @@ static int32_t get_time_diff(struct timeval *tv)
> return hsec;
> }
>
> +static void remove_timeouts(GDHCPClient *dhcp_client)
> +{
> +
> + if (dhcp_client->timeout > 0)
> + g_source_remove(dhcp_client->timeout);
> + if (dhcp_client->t1_timeout > 0)
> + g_source_remove(dhcp_client->t1_timeout);
> + if (dhcp_client->t2_timeout > 0)
> + g_source_remove(dhcp_client->t2_timeout);
> + if (dhcp_client->lease_timeout > 0)
> + g_source_remove(dhcp_client->lease_timeout);
> +
> + dhcp_client->timeout = 0;
> + dhcp_client->t1_timeout = 0;
> + dhcp_client->t2_timeout = 0;
> + dhcp_client->lease_timeout = 0;
> +
> +
Nitpick: unnecessary empty line here.
> +}
> +
> static void add_dhcpv6_request_options(GDHCPClient *dhcp_client,
> struct dhcpv6_packet *packet,
> unsigned char *buf, int max_buf,
> @@ -574,9 +597,7 @@ static gboolean send_announce_packet(gpointer dhcp_data)
> dhcp_client->requested_ip,
> dhcp_client->ifindex);
>
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> - dhcp_client->timeout = 0;
> + remove_timeouts(dhcp_client);
>
> if (dhcp_client->state == IPV4LL_DEFEND) {
> dhcp_client->timeout =
> @@ -1352,10 +1373,7 @@ static void ipv4ll_start(GDHCPClient *dhcp_client)
> guint timeout;
> int seed;
>
> - if (dhcp_client->timeout > 0) {
> - g_source_remove(dhcp_client->timeout);
> - dhcp_client->timeout = 0;
> - }
> + remove_timeouts(dhcp_client);
>
> switch_listening_mode(dhcp_client, L_NONE);
> dhcp_client->retry_times = 0;
> @@ -1381,8 +1399,7 @@ static void ipv4ll_stop(GDHCPClient *dhcp_client)
>
> switch_listening_mode(dhcp_client, L_NONE);
>
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> + remove_timeouts(dhcp_client);
>
> if (dhcp_client->listener_watch > 0) {
> g_source_remove(dhcp_client->listener_watch);
> @@ -1633,10 +1650,7 @@ static void restart_dhcp(GDHCPClient *dhcp_client, int
> retry_times)
> {
> debug(dhcp_client, "restart DHCP (retries %d)", retry_times);
>
> - if (dhcp_client->timeout > 0) {
> - g_source_remove(dhcp_client->timeout);
> - dhcp_client->timeout = 0;
> - }
> + remove_timeouts(dhcp_client);
>
> dhcp_client->retry_times = retry_times;
> dhcp_client->requested_ip = 0;
> @@ -1646,91 +1660,106 @@ static void restart_dhcp(GDHCPClient *dhcp_client,
> int retry_times)
> g_dhcp_client_start(dhcp_client, dhcp_client->last_address);
> }
>
> -static gboolean start_rebound_timeout(gpointer user_data)
> +static gboolean start_expire(gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
>
> - debug(dhcp_client, "start rebound timeout");
> + debug(dhcp_client, "lease expired");
>
> - switch_listening_mode(dhcp_client, L2);
> + //remove any timeouts if they are set
Nitpick: use /* */ for comments.
> - dhcp_client->lease_seconds >>= 1;
> + remove_timeouts(dhcp_client);
>
> - /* We need to have enough time to receive ACK package*/
> - if (dhcp_client->lease_seconds <= 6) {
>
> - /* ip need to be cleared */
> - if (dhcp_client->lease_lost_cb)
> - dhcp_client->lease_lost_cb(dhcp_client,
> - dhcp_client->lease_lost_data);
> + /* ip need to be cleared */
> + if (dhcp_client->lease_lost_cb)
> + dhcp_client->lease_lost_cb(dhcp_client,
> + dhcp_client->lease_lost_data);
>
> - restart_dhcp(dhcp_client, 0);
> - } else {
> - send_rebound(dhcp_client);
> + restart_dhcp(dhcp_client, 0);
>
> - dhcp_client->timeout =
> - g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_rebound_timeout,
> - dhcp_client,
> - NULL);
> + return false;
> +}
> +
> +static gboolean continue_rebound(gpointer user_data)
> +{
> + GDHCPClient *dhcp_client = user_data;
> +
> + switch_listening_mode(dhcp_client, L2);
> + send_rebound(dhcp_client);
> +
> + if (dhcp_client->t2_timeout> 0)
> + g_source_remove(dhcp_client->t2_timeout);
> +
> + //recalculate remaining rebind time
> + dhcp_client->T2 >>= 1;
> + if (dhcp_client->T2 > 60)
> + {
> + dhcp_client->t2_timeout =
> + g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->T2,
> + continue_rebound,
> + dhcp_client,
> + NULL);
It's not useful to call _add_seconds_full here, a network full of
ConnMan DHCP clients need to distribute the load, especially as they use
NTP and thus have synchronized clocks as well. Use g_timeout_add_full
with a +-1s time window, see section 4.1 of that RFC.
> }
>
> return FALSE;
> }
> -
> -static void start_rebound(GDHCPClient *dhcp_client)
Nitpick: add empty line before function starts.
> +static gboolean start_rebound(gpointer user_data)
> {
> - debug(dhcp_client, "start rebound");
> + GDHCPClient *dhcp_client = user_data;
>
> + //remove renew timer
> + if (dhcp_client->t1_timeout > 0)
> + g_source_remove(dhcp_client->t1_timeout);
> +
> + debug(dhcp_client, "start rebound");
> dhcp_client->state = REBINDING;
>
> - dhcp_client->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_rebound_timeout,
> - dhcp_client,
> - NULL);
> + //calculate total rebind time
> + dhcp_client->T2 = dhcp_client->expire - dhcp_client->T2;
Half the remaining time here, see 4.4.5.
> +
> + //send the first rebound and reschedule
> + continue_rebound(user_data);
> +
> + return FALSE;
> }
>
> -static gboolean start_renew_request_timeout(gpointer user_data)
> +static gboolean continue_renew (gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
>
> - debug(dhcp_client, "renew request timeout");
> + switch_listening_mode(dhcp_client, L3);
> + send_renew(dhcp_client);
>
> - if (dhcp_client->no_lease_cb)
> - dhcp_client->no_lease_cb(dhcp_client,
> - dhcp_client->no_lease_data);
> + if (dhcp_client->t1_timeout > 0)
> + g_source_remove(dhcp_client->t1_timeout);
>
> - return false;
> -}
> + dhcp_client->T1 >>= 1;
>
> -static gboolean start_renew_timeout(gpointer user_data)
> + if (dhcp_client->T1 > 60)
> + {
> + dhcp_client->t1_timeout =
> g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->T1,
> + continue_renew,
> + dhcp_client,
> + NULL);
> + }
As above.
> +
> + return FALSE;
> +}
> +static gboolean start_renew(gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
>
> - debug(dhcp_client, "start renew timeout");
> -
> + debug(dhcp_client, "start renew");
> dhcp_client->state = RENEWING;
>
> - dhcp_client->lease_seconds >>= 1;
> -
> - switch_listening_mode(dhcp_client, L3);
> - if (dhcp_client->lease_seconds <= 60)
> - start_rebound(dhcp_client);
> - else {
> - send_renew(dhcp_client);
> -
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> + //calculate total renew period
> + dhcp_client->T1 = dhcp_client->T2 - dhcp_client->T1;
Half the remaining time as above.
> - dhcp_client->timeout =
> - g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - REQUEST_TIMEOUT,
> - start_renew_request_timeout,
> - dhcp_client,
> - NULL);
> - }
> + //send first renew and reschedule for half the remaining time.
> + continue_renew(user_data);
>
> return FALSE;
> }
> @@ -1741,12 +1770,30 @@ static void start_bound(GDHCPClient *dhcp_client)
>
> dhcp_client->state = BOUND;
>
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> + remove_timeouts(dhcp_client);
>
> - dhcp_client->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_renew_timeout, dhcp_client,
> + /*
> + *TODO: T1 and T2 should be set through options instead of
> + * defaults as they are here.
> + */
> +
> + dhcp_client->T1 = dhcp_client->lease_seconds >> 1;
> + dhcp_client->T2 = dhcp_client->lease_seconds * 0.875;
> + dhcp_client->expire = dhcp_client->lease_seconds;
> +
> + dhcp_client->t1_timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->T1,
> + start_renew, dhcp_client,
> + NULL);
> +
> + dhcp_client->t2_timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->T2,
> + start_rebound, dhcp_client,
> + NULL);
> +
> + dhcp_client->lease_timeout= g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->expire,
> + start_expire, dhcp_client,
> NULL);
> }
>
> @@ -2317,7 +2364,7 @@ static gboolean listener_event(GIOChannel *channel,
> GIOCondition condition,
> if (*message_type != DHCPOFFER)
> return TRUE;
>
> - g_source_remove(dhcp_client->timeout);
> + remove_timeouts(dhcp_client);
> dhcp_client->timeout = 0;
> dhcp_client->retry_times = 0;
>
> @@ -2336,9 +2383,7 @@ static gboolean listener_event(GIOChannel *channel,
> GIOCondition condition,
> if (*message_type == DHCPACK) {
> dhcp_client->retry_times = 0;
>
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> - dhcp_client->timeout = 0;
> + remove_timeouts(dhcp_client);
>
> dhcp_client->lease_seconds = get_lease(&packet);
>
> @@ -2358,8 +2403,7 @@ static gboolean listener_event(GIOChannel *channel,
> GIOCondition condition,
> } else if (*message_type == DHCPNAK) {
> dhcp_client->retry_times = 0;
>
> - if (dhcp_client->timeout > 0)
> - g_source_remove(dhcp_client->timeout);
> + remove_timeouts(dhcp_client);
>
> dhcp_client->timeout = g_timeout_add_seconds_full(
> G_PRIORITY_HIGH, 3,
> @@ -2773,10 +2817,7 @@ void g_dhcp_client_stop(GDHCPClient *dhcp_client)
> send_release(dhcp_client, dhcp_client->server_ip,
> dhcp_client->requested_ip);
>
> - if (dhcp_client->timeout > 0) {
> - g_source_remove(dhcp_client->timeout);
> - dhcp_client->timeout = 0;
> - }
> + remove_timeouts(dhcp_client);
>
> if (dhcp_client->listener_watch > 0) {
> g_source_remove(dhcp_client->listener_watch);
And remember to rebase to latest upstream, some refactoring around
send_* happened recently.
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
