Re: [PATCH 3/8] station: add handling for new NETCONFIG state

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

>> Another weirdness is that I think we're sending the d-bus reply after 
>> connecting to the AP, but before netconfig runs.
> Probably this as well... But I'm not sure we can really send it after netconfig 
> unless we want to add a timeout error or something. If netconfig takes a long 

We already should be disconnecting if netconfig fails.  It is part of the 
NETCONFIG_EVENT_FAILED handling this patch was touching actually.  I'm not sure 
how this was tested exactly, but Andrew was pretty adamant that we should be 
doing so and I thought his arguments were compelling enough.

> time DBus will be unhappy. IMO netconfig is somewhat of a special case in this 
> regard, and consumers of the API should be waiting on the connected state, not 
> only the DBus method return.

netconfig should take a max of a few seconds under normal circumstances. 
Capping the DHCP time might make sense anyway and I think there are even some 
TODO items inside ell/netconfig.c about this.

Anyhow, this is a smaller issue compared to the one above.

>>> @@ -2043,8 +2044,9 @@ static void station_netconfig_event_handler(enum 
>>> netconfig_event event,
>>> dbus_pending_reply(&station->connect_pending, reply);
>>>           }
>>>   -        if (L_IN_SET(station->state, STATION_STATE_CONNECTING,
>>> -                STATION_STATE_CONNECTING_AUTO))
>>> +        if (L_IN_SET(station->state, STATION_STATE_NETCONFIG,
>>> +                STATION_STATE_ROAMING, STATION_STATE_FT_ROAMING,
>>> +                STATION_STATE_FW_ROAMING))
>>
>> I understand why NETCONFIG state is in the set, but why the others?
> This was because if we are roaming and netconfig fails for some reason we want 
> to disconnect, right?

Well, AFAIK, the original intent of this piece was to discard secrets if the 
initial connection failed.  This is now covered by the NETCONFIG state which is 
roughly equivalent to CONNECTING.

The addition of roaming states modifies the original intent.  Hence why I'm 
wondering why this is being added?

>> Also, shouldn't this also cover the roaming states?  And do we still need this 
>> check given wiphy_work?  Does netconfig use wiphy work to make sure nothing 
>> tries to scan or go off-channel?
> Yes, we shouldn't scan/offchannel while roaming. And no netconfig doesn't uses 
> the wiphy queue, but it really should.
>>
>>>           return dbus_error_busy(message);
>>>         if (!l_dbus_message_get_arguments(message, "aq", &iter))
> So sounds like I opened up a can of worms here :) We only noticed these issues 
> cropping up recently because of extended server upgrade times, i.e. the DHCP 

Yep, there's always going to be nasty corner cases to deal with.  I suspect your 
earlier change to re-start netconfig exposed some of these inconsistencies as well.

> server was down for a long time. Clients roamed and if they didn't have the 
> recent changes to restart netconfig they'd be stuck connected without an IP. I 

Hence why I think it is better to treat this as an initial connection failure 
rather than pretending that we roamed and dealing with the consequences.

> then noticed some of these other limitations now, but at least currently being 
> unable to roam is better than having no IP and requiring physical attention.

Regards,
-Denis