Re: [RFC PATCH v7 03/22] af_vsock: separate receive data loop

[Stefano Garzarella]

On Tue, Mar 23, 2021 at 04:09:36PM +0300, Arseny Krasnov wrote:
> Move STREAM specific data receive logic to '__vsock_stream_recvmsg()'
> dedicated function, while checks, that will be same for both STREAM
> and SEQPACKET sockets, stays in 'vsock_connectible_recvmsg()' shared
> functions.
>
> Signed-off-by: Arseny Krasnov <arseny.krasnov@xxxxxxxxxxxxx>
> ---
> net/vmw_vsock/af_vsock.c | 116 ++++++++++++++++++++++-----------------
> 1 file changed, 67 insertions(+), 49 deletions(-)

I had already reviewed this in v5 and in v6 you reported the R-b tag.

Usually the tag gets removed if you make changes to the patch or the 
reviewer is no longer happy. But this doesn't seem to be the case.

So please keep the tags between versions :-)


Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 421c0303b26f..0bc661e54262 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1895,65 +1895,22 @@ static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait,
> 	return data;
> }
>
> -static int
> -vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> -			  int flags)
> +static int __vsock_stream_recvmsg(struct sock *sk, struct msghdr *msg,
> +				  size_t len, int flags)
> {
> -	struct sock *sk;
> -	struct vsock_sock *vsk;
> +	struct vsock_transport_recv_notify_data recv_data;
> 	const struct vsock_transport *transport;
> -	int err;
> -	size_t target;
> +	struct vsock_sock *vsk;
> 	ssize_t copied;
> +	size_t target;
> 	long timeout;
> -	struct vsock_transport_recv_notify_data recv_data;
> +	int err;
>
> 	DEFINE_WAIT(wait);
>
> -	sk = sock->sk;
> 	vsk = vsock_sk(sk);
> -	err = 0;
> -
> -	lock_sock(sk);
> -
> 	transport = vsk->transport;
>
> -	if (!transport || sk->sk_state != TCP_ESTABLISHED) {
> -		/* Recvmsg is supposed to return 0 if a peer performs an
> -		 * orderly shutdown. Differentiate between that case and when a
> -		 * peer has not connected or a local shutdown occured with the
> -		 * SOCK_DONE flag.
> -		 */
> -		if (sock_flag(sk, SOCK_DONE))
> -			err = 0;
> -		else
> -			err = -ENOTCONN;
> -
> -		goto out;
> -	}
> -
> -	if (flags & MSG_OOB) {
> -		err = -EOPNOTSUPP;
> -		goto out;
> -	}
> -
> -	/* We don't check peer_shutdown flag here since peer may actually shut
> -	 * down, but there can be data in the queue that a local socket can
> -	 * receive.
> -	 */
> -	if (sk->sk_shutdown & RCV_SHUTDOWN) {
> -		err = 0;
> -		goto out;
> -	}
> -
> -	/* It is valid on Linux to pass in a zero-length receive buffer.  This
> -	 * is not an error.  We may as well bail out now.
> -	 */
> -	if (!len) {
> -		err = 0;
> -		goto out;
> -	}
> -
> 	/* We must not copy less than target bytes into the user's buffer
> 	 * before returning successfully, so we wait for the consume queue to
> 	 * have that much data to consume before dequeueing.  Note that this
> @@ -2012,6 +1969,67 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> 	if (copied > 0)
> 		err = copied;
>
> +out:
> +	return err;
> +}
> +
> +static int
> +vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> +			  int flags)
> +{
> +	struct sock *sk;
> +	struct vsock_sock *vsk;
> +	const struct vsock_transport *transport;
> +	int err;
> +
> +	DEFINE_WAIT(wait);
> +
> +	sk = sock->sk;
> +	vsk = vsock_sk(sk);
> +	err = 0;
> +
> +	lock_sock(sk);
> +
> +	transport = vsk->transport;
> +
> +	if (!transport || sk->sk_state != TCP_ESTABLISHED) {
> +		/* Recvmsg is supposed to return 0 if a peer performs an
> +		 * orderly shutdown. Differentiate between that case and when a
> +		 * peer has not connected or a local shutdown occurred with the
> +		 * SOCK_DONE flag.
> +		 */
> +		if (sock_flag(sk, SOCK_DONE))
> +			err = 0;
> +		else
> +			err = -ENOTCONN;
> +
> +		goto out;
> +	}
> +
> +	if (flags & MSG_OOB) {
> +		err = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	/* We don't check peer_shutdown flag here since peer may actually shut
> +	 * down, but there can be data in the queue that a local socket can
> +	 * receive.
> +	 */
> +	if (sk->sk_shutdown & RCV_SHUTDOWN) {
> +		err = 0;
> +		goto out;
> +	}
> +
> +	/* It is valid on Linux to pass in a zero-length receive buffer.  This
> +	 * is not an error.  We may as well bail out now.
> +	 */
> +	if (!len) {
> +		err = 0;
> +		goto out;
> +	}
> +
> +	err = __vsock_stream_recvmsg(sk, msg, len, flags);
> +
> out:
> 	release_sock(sk);
> 	return err;
> --
> 2.25.1