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

Hi James,

> 
> So from an ELL perspective I'm fine adding a retries setter (though timeout 
> makes more intuitive sense IMO) but I do want to explain some issues I see with 
> utilizing this in IWD, and doing netconfig prior to the DBus method return.

I disagree here.  For the dhcp_client, max retries is a much clearer 'lever' to 
adjust than the overall timeout.  As you said earlier, with fuzzing the number 
of retries in a given period can vary.  If the upper layers want to enforce a 
given timeout, they can do so anyway.

> 
> Adding netconfig into the mix would greatly increase the likelihood of a DBus 
> method timeout in "semi-normal" cases depending on the retry limit chosen. I 
> understand the dilemma you explained above but I do think IWD should try its 
> best to stay within the default DBus method timeout of 25 seconds. Trying to do 

You'll have to explain why you make this assertion?  If DBus bindings being used 
don't allow setting an infinite timeout, get better bindings.  I don't see any 
need to force iwd into fitting into this arbitrary 25 second limit.  And anyway, 
Connect() already can invoke Agent methods, which can take arbitrarily long 
anyway.  So netconfig is the least of your problems.

> this while also doing netconfig seems difficult unless we really cut down the 
> allowed retries, which has is drawbacks as poor RF could easily drop a few 
> frames and we could quickly get up to 25 seconds with the nature of the DHCP 
> timeouts and if the wifi association took a while (multiple BSS retries).

Do you have any data / examples to back up this assertion?

> 
> I'm also not sure if a DBus method timeout is going to break or cause weird 
> behavior with Connman/NetworkManager etc. And even if its handled, what exactly 

Not if they're written properly.

> should they do in this case? Will DHCP complete? should they re-issue connect 
> which could return -EBUSY? What happens when IWD does reply and a timeout has 
> already occurred? It really opens up a lot of possibilities here.

That is why I said that timeout handling has to be handled by the application. 
Most I've seen in real life do this poorly since there is usually a fundamental 
misunderstanding of how D-Bus works under the hood.

In practice, locally generated timeouts like the one used by default in 
libdbus-1 are just a bad idea.  I have no clue why they were added originally. 
If you have a misbehaving service on your bus which leaks messages, you will end 
up clogging D-Bus at some point and only a service restart will fix that.  Your 
service always has to reply to the message.  If you trust your running-as-root 
services to do the right thing, then adding arbitrary timeouts just gets in the way.

The fact that the timeout happened is unknown to the dbus peer.  It will 
continue as if nothing happened, and if well-behaved will generate a reply 
message eventually.  This has to happen for proper functioning of D-Bus.  The 
dbus-daemon will track that reply and forward it to the caller.  The caller's 
dbus implementation will just drop it on the floor since the reply-to serial 
number is no longer on the pending list.

So your iwd client has only two choices:
1. Call Connect() with no timeout and trust iwd to do the right thing.
2. Handle timeouts by trying to abort the connection somehow.  Perhaps via 
Station.Disconnect().  But that has its own side-effects.
3. If a timeout happens, assume iwd service is dead and restart it.

Hint: Option 1 or 3 is preferred

Regards,
-Denis