Re: [RFC PATCH v5 03/19] af_vsock: separate receive data loop

[Stefano Garzarella]

On Thu, Feb 18, 2021 at 08:36:50AM +0300, Arseny Krasnov wrote:
> This moves STREAM specific data receive logic to dedicated function:
> '__vsock_stream_recvmsg()', while checks that will be same for both
> types of socket are in shared function: 'vsock_connectible_recvmsg()'.

I'm not a native speaker, but I would rewrite this message like this:

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.

Anyway the patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>

> Signed-off-by: Arseny Krasnov <arseny.krasnov@xxxxxxxxxxxxx>
> ---
> net/vmw_vsock/af_vsock.c | 116 ++++++++++++++++++++++-----------------
> 1 file changed, 67 insertions(+), 49 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 6cf7bb977aa1..d277dc1cdbdf 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1894,65 +1894,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
> @@ -2011,6 +1968,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