Re: [PATCH v2 2/2] Bluetooth: Override status if local user rejects pairing

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Jaganath,

On Fri, Jul 06, 2012, Jaganath Kanakkassery wrote:
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5a442b9..4fc3379 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1764,6 +1764,10 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
>  	bacpy(&rp.addr.bdaddr, &conn->dst);
>  	rp.addr.type = link_to_bdaddr(conn->type, conn->dst_type);
>  
> +	/* Override status if local device rejected pairing */
> +	if (conn->auth_rejected == true)
> +		status = MGMT_STATUS_REJECTED;

I think simply "if (conn->auth_rejected)" should be fine (no "== true").
And what if status == 0 and this is a repairing over the same hci_conn
which was previously rejected? Seems like you'd give a false negative in
that case. Maybe the check should be "if (status && conn->auth_rejected)".

> +	/* Override status if local device rejected pairing */
> +	if (auth_rejected == true)

Same thing again with the comparison. The stuff inside () of an
if-statement should be a valid boolean, and if the standard bool type by
itself can't be considered that then I don't know what can.

The thing that's worrying me is that there's nowhere where you clear
conn->auth_rejected. If a re-authentication is attempted with the same
hci_conn the code would be doing wrong things. I'm not completely sure
where this clearing should occur since we're potentially sending two
mgmt events through two different code paths (pairing_complete and
mgmt_auth_failed) so clearing in either one might be risky in that it
causes the second function to do the wrong thing.

Johan