Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

On Thu, Apr 13, 2017 at 09:47:08PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 13, 2017 at 05:18:11PM +0100, Stefan Hajnoczi wrote:
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index af087b4..aae60c1 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/virtio_vsock.h>
> > +#include <uapi/linux/vsockmon.h>
> >  
> >  #include <net/sock.h>
> >  #include <net/af_vsock.h>
> > @@ -85,6 +86,63 @@ virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
> >  	return NULL;
> >  }
> >  
> > +/* Packet capture */
> > +void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > +	struct sk_buff *skb;
> > +	struct af_vsockmon_hdr *hdr;
> > +	unsigned char *t_hdr, *payload;
> > +
> > +	skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> > +			GFP_ATOMIC);
> > +	if (!skb)
> > +		return; /* nevermind if we cannot capture the packet */
> > +
> > +	hdr = (struct af_vsockmon_hdr *)skb_put(skb, sizeof(*hdr));
> > +
> > +	/* pkt->hdr is little-endian so no need to byteswap here */
> 
> Comment does not seem to make sense. Drop it?

All fields in pkt->hdr are little-endian.  All fields in the
af_vsockmon_hdr are little-endian too.

Normally code operating on either of these structs byteswaps the fields.
Therefore it seemed worth a comment to clarify that it's okay to omit
byteswaps.  The comment will help anyone auditing the code for
endianness bugs.

Why do you say it doesn't make sense?

> > +	hdr->src_cid = pkt->hdr.src_cid;
> > +	hdr->src_port = pkt->hdr.src_port;
> > +	hdr->dst_cid = pkt->hdr.dst_cid;
> > +	hdr->dst_port = pkt->hdr.dst_port;
> > +
> > +	hdr->transport = cpu_to_le16(AF_VSOCK_TRANSPORT_VIRTIO);
> > +	hdr->len = cpu_to_le16(sizeof(pkt->hdr));
> > +	hdr->reserved[0] = hdr->reserved[1] = 0;
> > +
> > +	switch(cpu_to_le16(pkt->hdr.op)) {
> 
> I'd expect this to be le16_to_cpu.
> Does this pass check build?

You are right, make C=2 warns about this and it should be le16_to_cpu().
I'll run check builds from now on.

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