Re: [RFC PATCH v4 03/17] af_vsock: separate receive data loop

[Stefano Garzarella]

On Sun, Feb 07, 2021 at 06:15:05PM +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()'.
>
> Signed-off-by: Arseny Krasnov <arseny.krasnov@xxxxxxxxxxxxx>
> ---
> net/vmw_vsock/af_vsock.c | 117 +++++++++++++++++++++++----------------
> 1 file changed, 68 insertions(+), 49 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 38927695786f..66c8a932f49b 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1898,65 +1898,22 @@ static int vsock_wait_data(struct sock *sk, struct wait_queue_entry *wait,
> 	return err;
> }
>
> -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

At the end of __vsock_stream_recvmsg() you are calling release_sock(sk) 
and it's wrong since we are releasing it in vsock_connectible_recvmsg().

Please fix it.

> @@ -2020,6 +1977,68 @@ vsock_connectible_recvmsg(struct socket *sock, 
> struct msghdr *msg, size_t len,
> 	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;
> +}
> +

The rest of the patch LGTM.

Stefano