Re: [PATCH v2] dhcp: fix overflow causing retries to stop

Hi James,

On 1/14/24 10:12, James Prestwood wrote:
> Hi Denis,
> 
> On 1/12/24 9:11 AM, Denis Kenzior wrote:
>> Hi James,
>>
>> On 1/11/24 07:33, James Prestwood wrote:
>>> If DHCP is in a SELECTING/REQUESTING state and the number of attempts
>>> reached a value where 2 << attempts overflowed an unsigned int the
>>> next timeout would become zero, causing DHCP to never retry without
>>> any event notification.
>>>
>>> Roughly 2 minutes has passed when we reach >5 attempts so it makes
>>> little sense to retry indefinitely, at least without notifying the
>>> upper layers (which could decided to retry themselves).
>>
>> Okay, but '5' is just a magic number in the code with no context. Lets avoid 
>> that.
>>
>> Can we make this limit configurable?  Say between 3..30?  For iwd, even 2 
>> minutes might be too long.
> 
> I'm thinking it may be better to leave the default unlimited retries but add a 
> configurable time limit versus retry limit. Setting based on retries isn't 
> intuitive a) because 99% of people have no idea what the backoff algorithm is, 
> and b) time limits are fuzzed so its not an exact amount of time.

Hopefully we know enough to tune the retries :)

> 
> I say this because if we want to include netconfig into IWD's Connect() DBus 
> method return we kinda need a way to define a time limit as opposed to guessing 
> how long 2-3 retries will actually take. We, potentially, could iterate through 
> several BSS's and fail extending the time to connect, then start DHCP. We would 
> need a way to limit netconfig to within the DBus method timeout (25 seconds 

DBus method timeouts don't exist on the protocol level.  ell doesn't even let 
you specify a method timeout for that reason.  Some libraries use a locally 
generated timeout by default (such as libdbus-1), but such libraries usually 
provide a way to specify an arbitrary timeout, or even disable it.

The problem with such timeouts is that when a library-local timeout is hit, 
creating a fake dbus method reply, there's no way to tell the callee.  In the 
case of iwd, it will just continue its Connect() operation since nothing 
happened as far as it is concerned.  Any dbus method timeouts are up to the 
application to deal with correctly...

Also, having dhcp use its own timeout incurs a cost of an extra file-descriptor. 
  Not worth it.

> IIRC), including how long all the connect attempts took prior. Maybe I'm 
> overthinking it but theoretically we could hit the method timeout limit so we 
> may want to add handling for it, which would be easier if we did the above with 
> a timeout versus retries.

All we can do is document that Connect might be a long-running operation (should 
be obvious) for the reasons you outline.

Regards,
-Denis