Re: [PATCH 3/3] netdev: Separate connect_failed and disconnected paths

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 11/14/23 16:05, Denis Kenzior wrote:
> Commit c59669a366c5 ("netdev: disambiguate between disconnection types")
> introduced different paths for different types of disconnection
> notifications from netdev.  Formalize this further by having
> netdev_connect_failed only invoke connect_cb.
>
> Disconnections that could be triggered outside of connection
> related events are now handled on a different code path.  For this
> purpose, netdev_disconnected() is introduced.
> ---
>   src/netdev.c | 45 +++++++++++++++++++++++++++++++++------------
>   1 file changed, 33 insertions(+), 12 deletions(-)

LGTM. You may have already thought of this, but I do still think we 
could have a case where an AP could send a deauth during key setting 
right?. So key setting could still succeed but station would get 
notified of the failure via the netdev event (in 
netdev_disconnect_event), not the connect callback.

> diff --git a/src/netdev.c b/src/netdev.c
> index 327be768d3d9..e31a51617671 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -839,7 +839,6 @@ static void netdev_connect_failed(struct netdev *netdev,
>   					uint16_t status_or_reason)
>   {
>   	netdev_connect_cb_t connect_cb = netdev->connect_cb;
> -	netdev_event_func_t event_filter = netdev->event_filter;
>   	void *connect_data = netdev->user_data;
>   
>   	/* Done this way to allow re-entrant netdev_connect calls */
> @@ -847,15 +846,6 @@ static void netdev_connect_failed(struct netdev *netdev,
>   
>   	if (connect_cb)
>   		connect_cb(netdev, result, &status_or_reason, connect_data);
> -	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;
> -
> -		event_filter(netdev, NETDEV_EVENT_DISCONNECT_BY_SME,
> -				&status_or_reason,
> -				connect_data);
> -	}
>   }
>   
>   static void netdev_connect_failed_cb(struct l_genl_msg *msg, void *user_data)
> @@ -900,12 +890,42 @@ static void netdev_deauth_and_fail_connection(struct netdev *netdev,
>   	netdev_send_and_fail_connection(netdev, result, status_code, msg);
>   }
>   
> +/*
> + * If we have a connection callback pending, either through netdev_connect
> + * or netdev_reassociate, then invoke that callback with the @result and
> + * @status_or_reason.  Otherwise, invoke the event callback with the @event
> + * and @status_or_reason.
> + *
> + * This is useful for situations where handshaking or setting keys somehow
> + * fails (perhaps due to rekeying), or if the device is removed / brought
> + * down when keys are being set as a result of a rekey
> + */
> +static void netdev_disconnected(struct netdev *netdev,
> +					enum netdev_result result,
> +					enum netdev_event event,
> +					uint16_t status_or_reason)
> +{
> +	netdev_event_func_t event_filter = netdev->event_filter;
> +	void *event_data = netdev->user_data;
> +
> +	if (netdev->connect_cb) {
> +		netdev_connect_failed(netdev, result, status_or_reason);
> +		return;
> +	}
> +
> +	netdev_connect_free(netdev);
> +
> +	if (event_filter)
> +		event_filter(netdev, event, &status_or_reason, event_data);
> +}
> +
>   static void netdev_disconnect_by_sme_cb(struct l_genl_msg *msg, void *user_data)
>   {
>   	struct netdev *netdev = user_data;
>   
>   	netdev->disconnect_cmd_id = 0;
> -	netdev_connect_failed(netdev, netdev->result, netdev->last_code);
> +	netdev_disconnected(netdev, netdev->result,
> +			NETDEV_EVENT_DISCONNECT_BY_SME, netdev->last_code);
>   }
>   
>   static void netdev_disconnect_by_sme(struct netdev *netdev,
> @@ -1440,7 +1460,8 @@ static void netdev_setting_keys_failed(struct netdev_handshake_state *nhs,
>   		 * CMD_DISCONNECT
>   		 */
>   		if (err == -ENETDOWN) {
> -			netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
> +			netdev_disconnected(netdev, NETDEV_RESULT_ABORTED,
> +					NETDEV_EVENT_DISCONNECT_BY_SME,
>   					MMPDU_STATUS_CODE_UNSPECIFIED);
>   			return;
>   		}