Ben, thank you so much for your comments, I have fixed your comments, moved some changes in this patch to patch 1, fixed compiling warnings and aligned cast errors, new version v4 has been posted out.
https://mail.openvswitch.org/pipermail/ovs-dev/2017-August/336864.html -----Original Message----- From: Ben Pfaff [mailto:[email protected]] Sent: Saturday, August 5, 2017 4:39 AM To: Yang, Yi Y <[email protected]> Cc: [email protected]; Jan Scheurich <[email protected]> Subject: Re: [PATCH v3 5/6] Generic encap and decap support for NSH On Thu, Aug 03, 2017 at 09:14:59AM +0800, Yi Yang wrote: > From: Jan Scheurich <[email protected]> > > This commit adds translation and netdev datapath support for generic > encap and decap actions for the NSH MD1 header. The generic encap and > decap actions are mapped to specific encap_nsh and decap_nsh actions > in the datapath. > > The translation follows that general scheme that decap() of an NSH > packet triggers recirculation after decapsulation, while encap(nsh) > just modifies struct flow and sets the ctx->pending_encap flag to > generate the encap_nsh action at the next commit to be able to include > subsequent set_field actions for NSH headers. > > Support for the flexible MD2 format using TLV properties is foreseen > in encap(nsh), but not yet fully implemented. > > The CLI syntax for encap of NSH is > encap(nsh(md_type=1)) > encap(nsh(md_type=2[,tlv(<tlv_class>,<tlv_type>,<hex_string>),...])) > > Signed-off-by: Jan Scheurich <[email protected]> > Signed-off-by: Yi Yang <[email protected]> I don't see the new parts added to openvswitch.h in upstream Linux (even in net-next), so I think that all of them should be enclosed in #ifndef __KERNEL__ to make that clear. struct ovs_action_encap_nsh is absurdly large due to the metadata array. It does not make sense for it to be that big given only MD1 support. Ideally, struct ovs_action_encap_nsh would be converted into nested Netlink attributes; then, the metadata could be variable length. That is the right way to add kernel support. Before kernel support, though, it would make more sense to just use a __be32[4] metadata array. This patch seems to make a lot of changes that should have been made in whatever patch originally added the code. For example, all the changes to format_nsh_key() and format_be32_masked() appear to be in this category. There are some new compiler warnings or errors: ../ofproto/ofproto-dpif-ipfix.c:2793:17: error: enumeration values 'OVS_ACTION_ATTR_ENCAP_NSH' and 'OVS_ACTION_ATTR_DECAP_NSH' not explicitly handled in switch [-Werror,-Wswitch-enum] ../ofproto/ofproto-dpif-xlate.c:5824:43: error: cast from 'const char *' to 'struct ofpact_ed_prop *' increases required alignment from 1 to 2 [-Werror,-Wcast-align] ../lib/odp-util.c:1847:21: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 [-Werror,-Wcast-align] ../lib/odp-util.c:6818:35: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'struct nsh_md1_ctx *' increases required alignment from 1 to 4 [-Werror,-Wcast-align] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
