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.

>
> > 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(),
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.

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
never uses netdev->connect_cb.  Perhaps netdev_disconnect_event()
should use netdev_connect_failed() instead.

>
> >               event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
> >                               &status_or_reason,
> >                               connect_data);
> > +     }
> >   }
> >
> >   static void netdev_disconnect_cb(struct l_genl_msg *msg, void *user_data)
> >
>
> Anyway, I applied this with a modified commit description.

Thanks.

Best regards