| Message ID | 20220224100641.2449550-1-gavin@matician.com |
|---|---|
| State | Accepted |
| Commit | f09ab95cfdb43a17dc97dd124b5a0b445a65086b |
| Headers | show |
| Series | Bluetooth: fix incorrect nonblock bitmask in bt_sock_wait_ready() | expand |
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
Hi Gavin, >>> 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. while that might be true, I find it a lot clearer if the variable is 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. And other code in the kernel makes sure to clearly turn something into a bool. You get 0x00 and 0x40. Regards Marcel
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index a647e5fabdbd6..87f0bba39b0f7 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -343,7 +343,7 @@ int bt_sock_stream_recvmsg(struct socket *sock, struct msghdr *msg, __poll_t bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait); int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg); int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo); -int bt_sock_wait_ready(struct sock *sk, unsigned long flags); +int bt_sock_wait_ready(struct sock *sk, unsigned int flags); void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh); void bt_accept_unlink(struct sock *sk); 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) { DECLARE_WAITQUEUE(wait, current); unsigned long timeo; @@ -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); add_wait_queue(sk_sleep(sk), &wait); set_current_state(TASK_INTERRUPTIBLE);