On 5/11/21 4:56 PM, Andrew Zaborowski wrote:
> On Tue, 11 May 2021 at 18:36, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 5/10/21 5:12 AM, Andrew Zaborowski wrote:
>>> Station callbacks expect a reason code (as opposed to status codes) with
>>> this event type.  This shouldn't matter a lot because
>>> station_disconnect_event() only actually looks at the value during
>>
>> I'm not sure why station is written this way.  I'm pretty sure that calling
>> station_disconnect_event() for NETDEV_EVENT_DISCONNECT_BY_SME() is the wrong
>> thing to do.
> 
> That is currently the only use of that function though.

No, we have DISCONNECT_BY_AP and DISCONNECT_BY_SME.  DISCONNECT_BY_SME should 
pretty much imply station_disassociated() should be called since we should never 
receive this event when we're in the connecting state.

Receiving DISCONNECT_BY_AP needs to be treated differently as well, depending on 
whether we're connected or connecting, regardless of connect_pending state (i.e. 
during autoconnect).  Or have netdev.c invoke connect_cb if we're still 
connecting instead of sending this event.

> 
>>
>>> connection establishment so usually the "result" parameter is going to
>>> be HANDSHAKE_FAILED meaning that we'd already be getting a reason code
>>> in status_or_reason, but there could be corner cases where we got a
>>> different "result".
>>
>> That is correct for AP disconnections during handshaking, but that is signaled
>> by NETDEV_EVENT_DISCONNECT_BY_AP...
>>
>>> ---
>>>    src/netdev.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/netdev.c b/src/netdev.c
>>> index 4fbe813a..6c063df5 100644
>>> --- a/src/netdev.c
>>> +++ b/src/netdev.c
>>> @@ -771,10 +771,15 @@ static void netdev_connect_failed(struct netdev *netdev,
>>>
>>>        if (connect_cb)
>>>                connect_cb(netdev, result, &status_or_reason, connect_data);
>>> -     else if (event_filter)
>>> +     else if (event_filter) {
>>> +             /* NETDEV_EVENT_DISCONNECT_BY_SME expects a reason code */
>>> +             if (result != NETDEV_RESULT_HANDSHAKE_FAILED)
>>> +                     status_or_reason = MMPDU_REASON_CODE_UNSPECIFIED;
>>> +
>>
>>   From what I remember, the only time we can get here is if a rekey somehow
>> fails, or setting keys during a rekey fails.  Are there other cases?
> 
> In theory you can pass a null connect_cb to netdev_connect_common(),

Yes, so either netdev should handle this specially, or we should make it an 
error condition during netdev_connect().

> but in practice we don't do this so you could trigger this only for:
> 
> NETDEV_RESULT_HANDSHAKE_FAILED,
> NETDEV_RESULT_KEY_SETTING_FAILED,
> NETDEV_RESULT_ABORTED.

NETDEV_ABORTED should only be invoked in the connecting phase, no?  I suppose 
there may be some bizarre situations, like we're rekeying and the device goes down.

> 
> I realize I was confused, I thought that netdev_connect_failed() was
> actually sending NETDEV_EVENT_DISCONNECT_BY_SME to station and
> triggering my autoconnect loop, but actually it was
> netdev_disconnect_event() which always calls netdev->event_filter and

This confirms my suspicions that we're not handling DISCONNECT_BY_AP completely 
correctly.

> never uses netdev->connect_cb.  Perhaps netdev_disconnect_event()
> should use netdev_connect_failed() instead.

They're different conditions though.  DISCONNECT_BY_AP tells us the AP sent a 
Deauthenticate.  DISCONNECT_BY_SME says that we did (actual or implied).

Regards,
-Denis