Ben, vxlangpe.h is from Linux kernel header file, so style is Linux but not ovs, keeping these intact is to make sure there are same pieces in both userspace and kernel, so code is also almost same. We'll change it to ovs style per your comments. Zoltan, please let me know if you need any help from me.
-----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Ben Pfaff Sent: Friday, May 19, 2017 12:54 PM To: Zoltán Balogh <[email protected]> Cc: '[email protected]' <[email protected]> Subject: Re: [ovs-dev] [PATCH v6 5/5] userspace: add vxlan gpe support to vport On Fri, May 12, 2017 at 11:07:46AM +0000, Zoltán Balogh wrote: > From: Georg Schmuecking <[email protected]> > > This patch is based on the "datapath: enable vxlangpe creation in compat mode" > from Yi Yang. It introduces an extension option "gpe" to the vxlan > port in the netdev-dpdk datapath. Furthermore it introduces a new > protocol header file vxlangpe.h in which the vxlan gpe protocoll is > described. In the vxlan specific methods the different packet are introduced > and handled. > > Added VXLAN GPE tunnel push test. > > Signed-off-by: Yi Yang <yi.y.yang at intel.com> > Signed-off-by: Georg Schmuecking <[email protected]> checkpatch says: ERROR: Inappropriate bracing around statement #138 FILE: lib/netdev-native-tnl.c:519: if (gpe->oam_flag) WARNING: Line has non-spaces leading whitespace #153 FILE: lib/netdev-native-tnl.c:534: } WARNING: Line length is >79-characters long #188 FILE: lib/netdev-native-tnl.c:578: put_16aligned_be32(&vxh->vx_vni, htonl(ntohll(params->flow->tunnel.tun_id) << 8)); WARNING: Line length is >79-characters long #206 FILE: lib/netdev-native-tnl.c:596: put_16aligned_be32(&vxh->vx_vni, htonl(ntohll(params->flow->tunnel.tun_id) << 8)); In netdev_vxlan_pop_header(), I don't see anything that verifies that the packet is long enough when GPE is present. The formatting of the switch statements look different from the usual OVS style, which is like this: switch (gpe->next_protocol) { case VXLAN_GPE_NP_IPV4: next_pt = PT_IPV4; break; case VXLAN_GPE_NP_IPV6: next_pt = PT_IPV6; break; case VXLAN_GPE_NP_ETHERNET: next_pt = PT_ETH; break; default: goto err; } I suspect that struct vxlanhdr_gpe should use ovs_16aligned_be32 for the vx_vni member. I doubt that these #defines are a good idea, in vxlangpe.h: #define u8 uint8_t #define u32 uint8_t #define __be32 ovs_be32 struct vxlanhdr in lib/packets.h is pretty different in philosophy from struct vxlanhdr_gpe in vxlangpe.h. I'd suggest making the new structure more like the existing one; OVS doesn't really use bit-fields much and in my experience they do not make code much easier to deal with. The new struct vxlan_metadata doesn't seem to be used anywhere and I wonder whether it should be included. Thanks, Ben. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
