Re: [RFC] VSOCK: add support for MSG_PEEK

[Stefano Garzarella]

Hi Matias,
thanks for this patch!

Since this patch only concerns virtio_transport,
I'd use the 'vsock/virtio' prefix in the commit title:
"vsock/virtio: add support for MSG_PEEK"

Some comments below:

On Sun, Sep 22, 2019 at 05:48:27PM +0000, Matias Ezequiel Vara Larsen wrote:
> This patch adds support for MSG_PEEK. In such a case, packets are not
> removed from the rx_queue and credit updates are not sent.
> 
> Signed-off-by: Matias Ezequiel Vara Larsen <matiasevara@xxxxxxxxx>
> ---
>  net/vmw_vsock/virtio_transport_common.c | 59 +++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 94cc0fa..830e890 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -264,6 +264,59 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk,
>  }
>  
>  static ssize_t
> +virtio_transport_stream_do_peek(struct vsock_sock *vsk,
> +				struct msghdr *msg,
> +				size_t len)
> +{
> +	struct virtio_vsock_sock *vvs = vsk->trans;
> +	struct virtio_vsock_pkt *pkt;
> +	size_t bytes, off = 0, total = 0;
> +	int err = -EFAULT;
> +
> +	spin_lock_bh(&vvs->rx_lock);
> +

What about using list_for_each_entry() to cycle through the queued packets?

> +	if (list_empty(&vvs->rx_queue)) {
> +		spin_unlock_bh(&vvs->rx_lock);
> +		return 0;
> +	}
> +
> +	pkt = list_first_entry(&vvs->rx_queue,
> +			       struct virtio_vsock_pkt, list);
> +	do {

pkt->off contains the offset inside the packet where the unread data starts.
So here we should initialize 'off':

		off = pkt->off;

Or just use pkt->off later (without increasing it as in the dequeue).

> +		bytes = len - total;
> +		if (bytes > pkt->len - off)
> +			bytes = pkt->len - off;
> +
> +		/* sk_lock is held by caller so no one else can dequeue.
> +		 * Unlock rx_lock since memcpy_to_msg() may sleep.
> +		 */
> +		spin_unlock_bh(&vvs->rx_lock);
> +
> +		err = memcpy_to_msg(msg, pkt->buf + off, bytes);
> +		if (err)
> +			goto out;
> +
> +		spin_lock_bh(&vvs->rx_lock);
> +
> +		total += bytes;

Using list_for_each_entry(), here we can just do:
(or better, at the beginning of the cycle)

		if (total == len)
			break;

removing the next part...

> +		off += bytes;
> +		if (off == pkt->len) {
> +			pkt = list_next_entry(pkt, list);
> +			off = 0;
> +		}
> +	} while ((total < len) && !list_is_first(&pkt->list, &vvs->rx_queue));

...until here.

> +
> +	spin_unlock_bh(&vvs->rx_lock);
> +
> +	return total;
> +
> +out:
> +	if (total)
> +		err = total;
> +	return err;
> +}
> +
> +static ssize_t
>  virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
>  				   struct msghdr *msg,
>  				   size_t len)
> @@ -330,9 +383,9 @@ virtio_transport_stream_dequeue(struct vsock_sock *vsk,
>  				size_t len, int flags)
>  {
>  	if (flags & MSG_PEEK)
> -		return -EOPNOTSUPP;
> -
> -	return virtio_transport_stream_do_dequeue(vsk, msg, len);
> +		return virtio_transport_stream_do_peek(vsk, msg, len);
> +	else
> +		return virtio_transport_stream_do_dequeue(vsk, msg, len);
>  }
>  EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
>  

The rest looks good to me!

Thanks,
Stefano