diff mbox series

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

Message ID 20210207151508.804615-1-arseny.krasnov@kaspersky.com
State Superseded
Headers show
Series virtio/vsock: introduce SOCK_SEQPACKET support | expand

Commit Message

Arseny Krasnov Feb. 7, 2021, 3:15 p.m. UTC 
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@kaspersky.com>
---
 net/vmw_vsock/af_vsock.c | 117 +++++++++++++++++++++++----------------
 1 file changed, 68 insertions(+), 49 deletions(-)

Comments

Stefano Garzarella Feb. 11, 2021, 11:37 a.m. UTC | #1 
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@kaspersky.com>

>---

> 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
diff mbox series

Patch

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
@@ -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;
+}
+
 static const struct proto_ops vsock_stream_ops = {
 	.family = PF_VSOCK,
 	.owner = THIS_MODULE,