On Thu, Dec 10, 2015 at 10:17:07AM +0000, Alex Bennée wrote:
> Stefan Hajnoczi <stefa...@redhat.com> writes:
> 
> > From: Asias He <as...@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: signature.asc
Description: PGP signature

Reply via email to