[RFC PATCH net-next v6 01/14] af_vsock: generalize vsock_dgram_recvmsg() to all transports

[Arseniy Krasnov]

> @@ -1273,11 +1273,15 @@ static int vsock_dgram_connect(struct socket *sock,
>  int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>  			size_t len, int flags)
>  {
> +	struct vsock_skb_cb *vsock_cb;
>  #ifdef CONFIG_BPF_SYSCALL
>  	const struct proto *prot;
>  #endif
>  	struct vsock_sock *vsk;
> +	struct sk_buff *skb;
> +	size_t payload_len;
>  	struct sock *sk;
> +	int err;
>  
>  	sk = sock->sk;
>  	vsk = vsock_sk(sk);
> @@ -1288,7 +1292,43 @@ int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>  		return prot->recvmsg(sk, msg, len, flags, NULL);
>  #endif
>  
> -	return vsk->transport->dgram_dequeue(vsk, msg, len, flags);
> +	if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
> +		return -EOPNOTSUPP;
> +
> +	if (unlikely(flags & MSG_ERRQUEUE))
> +		return sock_recv_errqueue(sk, msg, len, SOL_VSOCK, 0);
> +
> +	/* Retrieve the head sk_buff from the socket's receive queue. */
> +	err = 0;
> +	skb = skb_recv_datagram(sk_vsock(vsk), flags, &err);
> +	if (!skb)
> +		return err;
> +
> +	payload_len = skb->len;
> +
> +	if (payload_len > len) {
> +		payload_len = len;
> +		msg->msg_flags |= MSG_TRUNC;
> +	}
> +
> +	/* Place the datagram payload in the user's iovec. */
> +	err = skb_copy_datagram_msg(skb, 0, msg, payload_len);
> +	if (err)
> +		goto out;
> +
> +	if (msg->msg_name) {
> +		/* Provide the address of the sender. */
> +		DECLARE_SOCKADDR(struct sockaddr_vm *, vm_addr, msg->msg_name);
> +
> +		vsock_cb = vsock_skb_cb(skb);

May be we can declare 'vsock_cb' here ? Reducing its scope.

> +		vsock_addr_init(vm_addr, vsock_cb->src_cid, vsock_cb->src_port);
> +		msg->msg_namelen = sizeof(*vm_addr);
> +	}
> +	err = payload_len;
> +
> +out:
> +	skb_free_datagram(&vsk->sk, skb);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(vsock_dgram_recvmsg);
>  


> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -610,6 +610,7 @@ vmci_transport_datagram_create_hnd(u32 resource_id,
>  
>  static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
>  {
> +	struct vsock_skb_cb *vsock_cb;
>  	struct sock *sk;
>  	size_t size;
>  	struct sk_buff *skb;
> @@ -637,10 +638,14 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
>  	if (!skb)
>  		return VMCI_ERROR_NO_MEM;
>  
> +	vsock_cb = vsock_skb_cb(skb);
> +	vsock_cb->src_cid = dg->src.context;
> +	vsock_cb->src_port = dg->src.resource;
>  	/* sk_receive_skb() will do a sock_put(), so hold here. */
>  	sock_hold(sk);
>  	skb_put(skb, size);
>  	memcpy(skb->data, dg, size);
> +	skb_pull(skb, VMCI_DG_HEADERSIZE);

Small suggestion: here we do:

1) skb_put(skb, size of entire datagram)
2) memcpy(entire datagram)
3) skb_pull(VMCI_DG_HEADERSIZE)

If we provide only data to the upper layer, we can do:
1) skb_put(dg->payload_size)
2) memcpy(dg->payload_size)

Also (I'm no expert in VMCI), i guess using dg->payload_size is safer
to know number of data bytes, instead of using VMCI_DG_HEADERSIZE.

WDYT?

>  	sk_receive_skb(sk, skb, 0);
>  
>  	return VMCI_SUCCESS;
> @@ -1731,59 +1736,6 @@ static int vmci_transport_dgram_enqueue(
>  	return err - sizeof(*dg);
>  }
>  

Thanks