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