Re: [RFC PATCH v2 01/13] af_vsock: implement 'vsock_wait_data()'.

[Stefano Garzarella]

On Fri, Jan 15, 2021 at 08:40:25AM +0300, Arseny Krasnov wrote:
> This adds 'vsock_wait_data()' function which is called from user's read
> syscall and waits until new socket data is arrived. It was based on code
> from stream dequeue logic and moved to separate function because it will
> be called both from SOCK_STREAM and SOCK_SEQPACKET receive loops.
>
> Signed-off-by: Arseny Krasnov <arseny.krasnov@xxxxxxxxxxxxx>
> ---
> net/vmw_vsock/af_vsock.c | 47 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index b12d3a322242..af716f5a93a4 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1822,6 +1822,53 @@ static int vsock_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> 	return err;
> }
>
> +static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait,
> +			   long timeout,
> +			   struct vsock_transport_recv_notify_data *recv_data,
> +			   size_t target)
> +{
> +	int err = 0;
> +	struct vsock_sock *vsk;
> +	const struct vsock_transport *transport;

Please be sure that here and in all of the next patches, you follow the 
"Reverse Christmas tree" rule followed in net/ for the local variable 
declarations (order variable declaration lines longest to shortest).

> +
> +	vsk = vsock_sk(sk);
> +	transport = vsk->transport;
> +
> +	if (sk->sk_err != 0 ||
> +	    (sk->sk_shutdown & RCV_SHUTDOWN) ||
> +	    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
> +		finish_wait(sk_sleep(sk), wait);
> +		return -1;
> +	}
> +	/* Don't wait for non-blocking sockets. */
> +	if (timeout == 0) {
> +		err = -EAGAIN;
> +		finish_wait(sk_sleep(sk), wait);
> +		return err;
> +	}
> +
> +	if (recv_data) {
> +		err = transport->notify_recv_pre_block(vsk, target, recv_data);
> +		if (err < 0) {
> +			finish_wait(sk_sleep(sk), wait);
> +			return err;
> +		}
> +	}
> +
> +	release_sock(sk);
> +	timeout = schedule_timeout(timeout);
> +	lock_sock(sk);
> +
> +	if (signal_pending(current)) {
> +		err = sock_intr_errno(timeout);
> +		finish_wait(sk_sleep(sk), wait);
> +	} else if (timeout == 0) {
> +		err = -EAGAIN;
> +		finish_wait(sk_sleep(sk), wait);
> +	}
> +

Since we are calling finish_wait() before return in all path, why not 
doing somethig like this:

out:
	finish_wait(sk_sleep(sk), wait);
> +	return err;
> +}

Then in the error paths you can do:

	err = XXX;
	goto out;

Thanks,
Stefano

> static int
> vsock_stream_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> --
> 2.25.1