[ovs-dev] [PATCH 1/4] Add OF actions for generic encap and decap

[Ben Pfaff]

On Fri, Jun 30, 2017 at 03:29:29PM +0000, Zoltán Balogh wrote:
> From: Jan Scheurich <jan.scheurich at ericsson.com>
> 
> This commit adds support for the OpenFlow actions generic encap
> and decap (as specified in ONF EXT-382) to the OVS control plane.
> 
> CLI syntax for encap action with properties:
>   encap(hdr=<header>)
>   encap(hdr=<header>,
>         prop(class=<class>,type=<type>,val=<simple>),
>         prop(class=<class>,type=<type>,val(<complex>)))
> 
> CLI syntax for decap action:
>   decap()
>   decap(packet_type(ns=<pt_ns>,type=<pt_type>))
> 
> The first header supported for encap and decap is "ethernet" to convert
> packets between packet_type (1,Ethertype) and (0,0).
> 
> Signed-off-by: Jan Scheurich <jan.scheurich at ericsson.com>
> Signed-off-by: Yi Yang <yi.y.yang at intel.com>

Thanks for working on this.  I have some comments.

The patch was partially corrupted: lines in the patch that should have
had a single space were changed to blank lines.  This prevented the
patch from applying without manual editing.  I was able to work around
it.  (If you use "git send-email", this problem does not happen.)

Please rename __OFPHTN_MAX to avoid the __ prefix, since that's reserved
to the C implementation.  Maybe OFPHTN_N_TYPES would be better.

GCC says:

    ../lib/ofp-actions.c:4189:13: error: cast from 'char *' to 'struct ofpact_encap *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
    ../lib/ofp-actions.c:4222:16: error: cast from 'const uint8_t *' (aka 'const unsigned char *') to 'const struct ofpact_ed_prop *' increases required alignment from 1 to 2 [-Werror,-Wcast-align]

ofp-actions.c is using OpenFlow action numbers 29 and 30, but there's no
standardization of those values.  I suggest using an NX extension number
instead, since we control those directly in OVS.

Same goes for the new OFPERR_OFPBAC_* errors.  You can use NX
extensions.

In three places I see [0] used for a flexible array member.  MSVC
rejects this.  You can use [] in place of [0].

In encode_ENCAP(), please put a space on each side of the binary
operators here:
+    for (i=0; i<encap->n_props; i++) {
Same in format_ed_props().

What is your plan for handling ofpact_check__() with decap actions?

This patch is incomplete since it doesn't actually implement the encap
or decap actions (in do_xlate_actions()).  Maybe that's in a later
patch.

Thanks,

Ben.