[En-Nut-Discussion] DHCP Failure on Renew

[Michael Jones]

Hello,

Ulrich wrote:

>How did you find out about that problem? Is there a backup DHCP in your 
>network and the primary failed? Or did you simply leave the box in you 
>network that long, that the lease expired? I think I remember, that the 
>EIR did not work after lying around a longer time playing musik. I 
>simply reset it and it worked again for days. But I never tried to find 
>out why and I don't know the lease-time the Fritbox assigns to it's 
>clients.

>I'll try that DHCP lease timeout in the next few days. Never checked 
>that, as my problems were at another situation.

Well, I simply setup an DHCP server with a 1 hour lease time - the system
crash after half an hour so no big challenge. 

As far as I can see the DHCP code is a little overcomplicated and the main
codes readability could have benefited from being stuck in a switch/case
construct. 

Any way; I found two problematic points in the code so far. The First, in
line 1708 will cause the DhcpBroadcastRequest call, which is essential for
keeping the state machine running, to not being called as at the time that
this is executed it is already '<=' leaseTime. 

So changing the condition to < might help. But also the DhcpBroadcastRequest
should, in my opinion, always be called to make sure the state machine does
not go stale - hence remove the 'else' in line 1714. Also, recalling
NutGetSeconds() again and again throughout the routine also seems like a
waste and induces a possible race condition as the second might just tick
over between to calls.


        else if (dhcpState == DHCPST_RENEWING) {
            retries++;
            if (tmo / 1000 > dhcpConfig->dyn_rebindTime - (NutGetSeconds() -
leaseTime)) {
                tmo = dhcpConfig->dyn_rebindTime - (NutGetSeconds() -
leaseTime) * 1000;
            }
1708:       if (dhcpConfig->dyn_rebindTime <= NutGetSeconds() - leaseTime) {
                retries = 0;
                dhcpState = DHCPST_REBINDING;
            }
            /* Send a request to our leasing server. We must not include
               the server identifier. */
            else if (DhcpSendRequest(sock, dhcpConfig->dyn_sid, bp, xid,
dhcpConfig->dyn_yiaddr, dhcpConfig->dyn_yiaddr, 0, secs) <
                     0) {
                /* Unicast transmit error. */
                retries = 0;
                dhcpState = DHCPST_REBINDING;


A bit lower we encounter the same. Here, I feel that the whole leaseTime
'if' is completely de-placed 


        else if (dhcpState == DHCPST_REBINDING) {
            retries++;
            if (tmo > dhcpConfig->dyn_rebindTime - (NutGetSeconds() -
leaseTime)) {
                tmo = dhcpConfig->dyn_rebindTime - NutGetSeconds() -
leaseTime;
            }
1746:       if (dhcpConfig->dyn_rebindTime <= NutGetSeconds() - leaseTime) {
                retries = 0;
                dhcpState = DHCPST_REBINDING;
            }
            /* Broadcast a request for our previous configuration. We 
               must not include a server identifier. */
            else if (DhcpBroadcastRequest(sock, bp, xid,
dhcpConfig->dyn_yiaddr, dhcpConfig->dyn_yiaddr, 0, secs) < 0) {
                /* Fatal transmit error on broadcast. Give up. */
                dhcpState = DHCPST_IDLE;

My current implementation with simply removing both 'else' works like a
charm and I have not had any negative side effects. But, my plan is fully
understand the routine and then to re-write and simplify the routine...

Cu,
Michael