I had a similar comment to avoid bit-fields in the nsh_hdr struct in the NSH patch series. @Yi: Have a look at the restructured nsh.h in my new branch.
I suggest you just add any required vxlan-gpe specific items to the VXLAN section in lib/packets.h rather than creating a new header file for vxlan-gpe. /Jan > -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Yang, Yi Y > Sent: Friday, 19 May, 2017 07:18 > To: Ben Pfaff <[email protected]>; 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 > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
