On Wed, 2014-01-22 at 14:20 -0800, Andrew LeCain wrote:
> Switching to using T1 and T2 rather than hardcoded limits. Defaulting
> T1 and T2 to 50% and 87.5%.
>
> Implementation now sends renew requests and waits for the appropriate
> timeout before resuming the exponential decay timeout as per spec. The
> same is true of rebind.
Hmm... a bit more details of the how part would be appreciated.
> ---
> gdhcp/client.c | 105
> ++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/gdhcp/client.c b/gdhcp/client.c
> index b24d19d..bb9a2e2 100644
> --- a/gdhcp/client.c
> +++ b/gdhcp/client.c
> @@ -1646,6 +1646,7 @@ static void restart_dhcp(GDHCPClient *dhcp_client, int
> retry_times)
> g_dhcp_client_start(dhcp_client, dhcp_client->last_address);
> }
>
> +static gboolean start_rebound(gpointer dhcp_client);
> static gboolean start_rebound_timeout(gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
> @@ -1654,10 +1655,10 @@ static gboolean start_rebound_timeout(gpointer
> user_data)
>
> switch_listening_mode(dhcp_client, L2);
>
> - dhcp_client->lease_seconds >>= 1;
> + dhcp_client->expire >>= 1;
>
> /* We need to have enough time to receive ACK package*/
> - if (dhcp_client->lease_seconds <= 6) {
> + if (dhcp_client->expire <= 60) {
>
> /* ip need to be cleared */
> if (dhcp_client->lease_lost_cb)
> @@ -1666,46 +1667,69 @@ static gboolean start_rebound_timeout(gpointer
> user_data)
>
> restart_dhcp(dhcp_client, 0);
> } else {
> - send_rebound(dhcp_client);
> -
> dhcp_client->timeout =
> g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_rebound_timeout,
> - dhcp_client,
> - NULL);
> + dhcp_client->expire,
> + start_rebound,
> + dhcp_client,
> + NULL);
> }
>
> - return FALSE;
> + return false;
> }
>
> -static void start_rebound(GDHCPClient *dhcp_client)
> +static gboolean start_rebound(gpointer user_data)
> {
> + GDHCPClient *dhcp_client = user_data;
> debug(dhcp_client, "start rebound");
>
> dhcp_client->state = REBINDING;
>
> + if (dhcp_client->timeout > 0){
> + g_source_remove(dhcp_client->timeout);
> + }
> +
> + dhcp_client->expire -= REQUEST_TIMEOUT;
> + send_rebound(dhcp_client);
> dhcp_client->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_rebound_timeout,
> - dhcp_client,
> - NULL);
> + REQUEST_TIMEOUT,
> + start_rebound_timeout,
> + dhcp_client,
> + NULL);
> +
> + return FALSE;
> }
>
> +static gboolean start_renew(gpointer user_data);
> static gboolean start_renew_request_timeout(gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
>
> - debug(dhcp_client, "renew request timeout");
>
> - if (dhcp_client->no_lease_cb)
> + dhcp_client->T2>>=1;
> +
> + if (dhcp_client->T2 <= 60)
The rebound timer should by default trigger at 7/8 of the lease, not
when there is less than 60 seconds remaining. Calling this function
happens after REQUEST_TIMEOUT from start_renew, which again is called
after dhcp_client->T1. Both of them are most likely unequal to ½T2.
> + {
> + debug(dhcp_client, "renew request timeout");
> + if (dhcp_client->no_lease_cb)
> dhcp_client->no_lease_cb(dhcp_client,
> - dhcp_client->no_lease_data);
> + dhcp_client->no_lease_data);
> +
> + start_rebound(dhcp_client);
> + } else {
> +
> + /*reschedule the renew for 1/2 the renew time remaining*/
> + dhcp_client->timeout =
> g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + dhcp_client->T2,
> + start_renew,
> + dhcp_client,
> + NULL);
> + }
How do we know whether T2 or the half time between T1 and lease
expiration is to be used for the next timeout here if we don't compare
them in any way?
> return false;
> }
>
> -static gboolean start_renew_timeout(gpointer user_data)
> +static gboolean start_renew(gpointer user_data)
> {
> GDHCPClient *dhcp_client = user_data;
>
> @@ -1713,24 +1737,20 @@ static gboolean start_renew_timeout(gpointer
> user_data)
>
> 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);
> + if (dhcp_client->timeout > 0)
> + g_source_remove(dhcp_client->timeout);
>
> - dhcp_client->timeout =
> - g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - REQUEST_TIMEOUT,
> - start_renew_request_timeout,
> - dhcp_client,
> - NULL);
> - }
> + send_renew(dhcp_client);
> +
> + dhcp_client->T2 -= REQUEST_TIMEOUT;
> + dhcp_client->timeout =
> + g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> + REQUEST_TIMEOUT,
> + start_renew_request_timeout,
> + dhcp_client,
> + NULL);
>
> return FALSE;
> }
> @@ -1744,9 +1764,20 @@ static void start_bound(GDHCPClient *dhcp_client)
> if (dhcp_client->timeout > 0)
> g_source_remove(dhcp_client->timeout);
>
> + /*
> + *TODO: T1 and T2 should be set through options instead of
> + * defaults as they are here, also note that the actual value
> + * of T2 is T1+T2. Because we don't start the T2 timer until
> + * T1 is elapsed we subtract it now while we know the original T1
> + */
> +
> + dhcp_client->T1 = dhcp_client->lease_seconds >> 1;
> + dhcp_client->T2 = dhcp_client->lease_seconds * 0.875 - dhcp_client->T1;
> + dhcp_client->expire = dhcp_client->lease_seconds - dhcp_client->T1 -
> dhcp_client->T2;
> +
> dhcp_client->timeout = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> - dhcp_client->lease_seconds >> 1,
> - start_renew_timeout, dhcp_client,
> + dhcp_client->T1,
> + start_renew, dhcp_client,
> NULL);
> }
It'd make the current implementation easier to follow and less to keep
track of if there were separate timers for T1, T2 and expiry.
> @@ -2583,7 +2614,7 @@ static gboolean ipv4ll_announce_timeout(gpointer
> dhcp_data)
> uint32_t ip;
>
> debug(dhcp_client, "request timeout (retries %d)",
> - dhcp_client->retry_times);
> + dhcp_client->retry_times);
>
> if (dhcp_client->retry_times != ANNOUNCE_NUM) {
> dhcp_client->retry_times++;
> @@ -2610,7 +2641,7 @@ static gboolean ipv4ll_probe_timeout(gpointer dhcp_data)
> GDHCPClient *dhcp_client = dhcp_data;
>
> debug(dhcp_client, "IPV4LL probe timeout (retries %d)",
> - dhcp_client->retry_times);
> + dhcp_client->retry_times);
>
> if (dhcp_client->retry_times == PROBE_NUM) {
> dhcp_client->state = IPV4LL_ANNOUNCE;
These two changes are just indentation fixes, should not hang around in
this patch.
Actually, the existing implementation for DHCPv4 is difficult to follow,
since it's using relative times and is actually not properly following
RFC 2131 what comes to T2 Rebound triggering. Following RFC 2131 closely
ends up with something:
- create separate T1, T2 and expiry timers
- when T1 triggers, recalculate T1, start renew
- when T2 triggers, start rebound, remove T1 timer completely
- when expiry triggers, remove T1 and T2 timers and report failure
- reading T1 and T2 from DHCP options is a good thing to have
implemented
- alternatively one can do the same relative computation as in DHCPv6
and use only one timer. It can be harder to follow, though.
- RFC 2131 is your friend, the current implementation is not that
helpful
When doing this, the current start_{renew,rebound}* functions may go
away to be replaced by needed timer callbacks (unless the same relative
computation as in DHCPv6 is used).
But anyway, thanks for biting into this!
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
