Re: [PATCH v3 1/4] VSOCK: Introduce virtio-vsock-common.ko

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4841 bytes --]

On Thu, Dec 10, 2015 at 10:17:07AM +0000, Alex Bennée wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > From: Asias He <asias@redhat.com>
> >
> > This module contains the common code and header files for the following
> > virtio-vsock and virtio-vhost kernel modules.
> 
> General comment checkpatch has a bunch of warnings about 80 character
> limits, extra braces and BUG_ON usage.

Will fix in the next verison.

> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > new file mode 100644
> > index 0000000..e54eb45
> > --- /dev/null
> > +++ b/include/linux/virtio_vsock.h
> > @@ -0,0 +1,203 @@
> > +/*
> > + * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
> > + * anyone can use the definitions to implement compatible
> > drivers/servers:
> 
> Is anything in here actually exposed to userspace or the guest? The
> #ifdef __KERNEL__ statement seems redundant for this file at least.

You are right.  I think the header was copied from a uapi file.

I'll compare against other virtio code and apply an appropriate header.

> > +void virtio_vsock_dumppkt(const char *func,  const struct virtio_vsock_pkt *pkt)
> > +{
> > +	pr_debug("%s: pkt=%p, op=%d, len=%d, %d:%d---%d:%d, len=%d\n",
> > +		 func, pkt,
> > +		 le16_to_cpu(pkt->hdr.op),
> > +		 le32_to_cpu(pkt->hdr.len),
> > +		 le32_to_cpu(pkt->hdr.src_cid),
> > +		 le32_to_cpu(pkt->hdr.src_port),
> > +		 le32_to_cpu(pkt->hdr.dst_cid),
> > +		 le32_to_cpu(pkt->hdr.dst_port),
> > +		 pkt->len);
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_vsock_dumppkt);
> 
> Why export this at all? The only users are in this file so you could
> make it static.

I'll make it static.

> > +u32 virtio_transport_get_credit(struct virtio_transport *trans, u32 credit)
> > +{
> > +	u32 ret;
> > +
> > +	mutex_lock(&trans->tx_lock);
> > +	ret = trans->peer_buf_alloc - (trans->tx_cnt - trans->peer_fwd_cnt);
> > +	if (ret > credit)
> > +		ret = credit;
> > +	trans->tx_cnt += ret;
> > +	mutex_unlock(&trans->tx_lock);
> > +
> > +	pr_debug("%s: ret=%d, buf_alloc=%d, peer_buf_alloc=%d,"
> > +		 "tx_cnt=%d, fwd_cnt=%d, peer_fwd_cnt=%d\n", __func__,
> 
> I think __func__ is superfluous here as the dynamic print code already
> has it and can print it when required. Having said that there seems to
> be plenty of code already in the kernel that uses __func__ :-/

I'll convert most printks to tracepoints in the next revision.

> > +u64 virtio_transport_get_max_buffer_size(struct vsock_sock *vsk)
> > +{
> > +	struct virtio_transport *trans = vsk->trans;
> > +
> > +	return trans->buf_size_max;
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_transport_get_max_buffer_size);
> 
> All these accesses functions seem pretty simple. Maybe they should be
> inline header functions or even #define macros?

They are used as struct vsock_transport function pointers.  What is the
advantage to inlining them?

> > +int virtio_transport_notify_send_post_enqueue(struct vsock_sock *vsk,
> > +	ssize_t written, struct vsock_transport_send_notify_data *data)
> > +{
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_transport_notify_send_post_enqueue);
> 
> This makes me wonder if the calling code should be having
> if(transport->fn) checks rather than filling stuff out will null
> implementations but I guess that's a question better aimed at the
> maintainers.

I've considered it too.  I'll try to streamline this in the next
revision.

> > +/* We are under the virtio-vsock's vsock->rx_lock or
> > + * vhost-vsock's vq->mutex lock */
> > +void virtio_transport_recv_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > +	struct virtio_transport *trans;
> > +	struct sockaddr_vm src, dst;
> > +	struct vsock_sock *vsk;
> > +	struct sock *sk;
> > +
> > +	vsock_addr_init(&src, le32_to_cpu(pkt->hdr.src_cid), le32_to_cpu(pkt->hdr.src_port));
> > +	vsock_addr_init(&dst, le32_to_cpu(pkt->hdr.dst_cid), le32_to_cpu(pkt->hdr.dst_port));
> > +
> > +	virtio_vsock_dumppkt(__func__, pkt);
> > +
> > +	if (le16_to_cpu(pkt->hdr.type) != VIRTIO_VSOCK_TYPE_STREAM) {
> > +		/* TODO send RST */
> 
> TODO's shouldn't make it into final submissions.
> 
> > +		goto free_pkt;
> > +	}
> > +
> > +	/* The socket must be in connected or bound table
> > +	 * otherwise send reset back
> > +	 */
> > +	sk = vsock_find_connected_socket(&src, &dst);
> > +	if (!sk) {
> > +		sk = vsock_find_bound_socket(&dst);
> > +		if (!sk) {
> > +			pr_debug("%s: can not find bound_socket\n", __func__);
> > +			virtio_vsock_dumppkt(__func__, pkt);
> > +			/* Ignore this pkt instead of sending reset back */
> > +			/* TODO send a RST unless this packet is a RST
> > (to avoid infinite loops) */
> 
> Ditto.

Thanks, I'll complete the RST code in the next revision.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization