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

Stefano Garzarella
2021-02-22T11:45:46Z

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@redhat.com>

>
>Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>---
> 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
>