On Thu, Dec 10, 2015 at 10:17:07AM +0000, Alex Bennée wrote:
> Stefan Hajnoczi <stefanha@xxxxxxxxxx> writes:
>
> > From: Asias He <asias@xxxxxxxxxx>
> >
> > 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
