On Wed, Nov 04, 2026 at 04:48:44PM +0000, [email protected] wrote:
On Fri, Oct 30, 2026 at 03:18:10PM +0100, Stefano Garzarella wrote:
Ditto about commit message, please put more effort on writing something
readable by a human.
Agreed, I'll rewrite both messages myself for v4, much shorter.
>+ if (sk->sk_err) {
>+ ret = -sk->sk_err;
Why?
Same as vsock_connectible_wait_data(): an error that arrives during the
wait is returned to recv(). Without the check the loop still ends, since
every path that sets sk_err also closes the socket or sets peer_shutdown,
but recv() then returns 0 or -ENOTCONN instead of the error.
Please add a comment with the reason.
>+ if (sk->sk_state != TCP_ESTABLISHED) {
>+ ret = sock_flag(sk, SOCK_DONE) ? 0 : -ENOTCONN;
>+ break;
>+ }
I'm thinking if we should split this in multiple patches.
From reading the code, it can be split without breaking bisect: first
loop on all exit conditions with the lock still held, then drop the lock
around wait_woken(). The order is fixed, since dropping the lock alone
brings back the early -EAGAIN, and the loop alone does not fix the
deadlock, so both would carry the Fixes tag. That is why I kept it as one
patch, but I'm fine either way. Split it like that for v4, with the
current 2/2 on top, or keep it as one?
Okay, if the entire change is needed to fix the deadlock, then it makes
sense to have a single patch. Please make it clear in the commit
description.
>+ if (!timeo) {
>+ ret = -EAGAIN;
>+ break;
>+ }
Is the move of this check changing some user visible error?
I mean before this patch it was one of the first checks, is this okay?
Yes, for non-blocking reads, intentionally: -sk_err, 0 on peer shutdown
and the not-connected result now come before -EAGAIN, as on the native
recvmsg path. I'll say so in the commit message.
Yes, please.
>+ if (signal_pending(current)) {
>+ ret = sock_intr_errno(timeo);
>+ break;
>+ }
Why? Should be a separate patch?
No, the loop needs it: with a pending signal wait_woken() returns at
once, so without the check the loop spins until SO_RCVTIMEO expires, or
forever if none is set. I checked it by removing the check: a signalled
reader without SO_RCVTIMEO never returned and used a full CPU. The
native path does the same.
Okay, please explain this in the commit description or in a comment on
top of that check.
>+ if (data < 0) {
Why not data <= 0 and void the other if?
Right, will do in v4.
Thanks,
Jerome
