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.
signature.asc
Description: PGP signature