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.
> >+ 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?
> >+ 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.
> >+ 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.
> >+ if (data < 0) {
>
> Why not data <= 0 and void the other if?
Right, will do in v4.
Thanks,
Jerome
