Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220224100641.2449550-1-gavin@matician.com> <71D25C8F-67D1-4EC0-9160-5F61C832F0AF@holtmann.org>
 <CACB127+s8mj2-Hkt4yZXRZ_cQ9CbAK0r3y709-8Cb3awpVJm9Q@mail.gmail.com>
In-Reply-To: <CACB127+s8mj2-Hkt4yZXRZ_cQ9CbAK0r3y709-8Cb3awpVJm9Q@mail.gmail.com>
From:   Gavin Li <gavin@matician.com>
Date:   Fri, 11 Mar 2022 05:39:58 -0800
Message-ID: <CACB127+XwPBWOL_VL7Xq+FvD5zFhn0ZUDURvVvV6OdN0oYpvUQ@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: fix incorrect nonblock bitmask in bt_sock_wait_ready()
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

Please let me know what you think with regards to the comments above.

Best,
Gavin


On Thu, Feb 24, 2022 at 12:56 PM Gavin Li <gavin@matician.com> wrote:
>
> Hi Marcel,
>
> Thanks for reviewing this quickly.
>
> > > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > > index ee319779781e6..69374321130e4 100644
> > > --- a/net/bluetooth/af_bluetooth.c
> > > +++ b/net/bluetooth/af_bluetooth.c
> > > @@ -568,7 +568,7 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> > > EXPORT_SYMBOL(bt_sock_wait_state);
> > >
> > > /* This function expects the sk lock to be held when called */
> > > -int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
> > > +int bt_sock_wait_ready(struct sock *sk, unsigned int flags)
> >
> > can we then also do s/flags/msg_flags/ then.
> I prefer keeping it as flags because all other net code also uses
> flags, msg_flags only appears
> in msg->msg_flags.
>
> > > @@ -576,7 +576,7 @@ int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
> > >
> > >       BT_DBG("sk %p", sk);
> > >
> > > -     timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> > > +     timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
> >
> > Since sock_sndtimeo() is taking a bool. This might be better !!(flags & MSG_DONTWAIT).
> It appears to be well-known in the net code that sock_sndtimeo takes a
> bool, since no other
> uses of it do the "!!" conversion.
>
> Let me know what you think. I can make the changes if needed but I was
> just trying my best
> to match the currently existing convention.
>
> Best,
> Gavin