[ovs-dev,2/5] ofp-actions: Translate OF1.0 "enqueue" actions for OF1.1+.

Previously, the OF1.0 "enqueue" action was simply omitted when actions
were translated into OpenFlow 1.1 or later, which do not have a similar
action.  This commit translates this action into an equivalent sequence
of actions.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 NEWS                 |  1 +
 lib/ofp-actions.c    |  8 +++++++-
 tests/ofp-actions.at | 17 +++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

With a small question below:

Acked-by: Jarno Rajahalme <jarno@ovn.org>

> On Jul 13, 2016, at 5:06 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Previously, the OF1.0 "enqueue" action was simply omitted when actions
> were translated into OpenFlow 1.1 or later, which do not have a similar
> action.  This commit translates this action into an equivalent sequence
> of actions.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> NEWS                 |  1 +
> lib/ofp-actions.c    |  8 +++++++-
> tests/ofp-actions.at | 17 +++++++++++++++++
> 3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/NEWS b/NEWS
> index b376420..3c206f7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -20,6 +20,7 @@ Post-v2.5.0
>      * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD message that adds a
>        new group or modifies an existing groups
>      * New OpenFlow extension to support the "group" action in OpenFlow 1.0.
> +     * OpenFlow 1.0 "enqueue" action now properly translated to OpenFlow 1.1+.
>    - ovs-ofctl:
>      * queue-get-config command now allows a queue ID to be specified.
>      * '--bundle' option can now be used with OpenFlow 1.3.
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 0aafe0a..4ac284f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -990,7 +990,13 @@ encode_ENQUEUE(const struct ofpact_enqueue *enqueue,
>         oae->port = htons(ofp_to_u16(enqueue->port));
>         oae->queue_id = htonl(enqueue->queue);
>     } else {
> -        /* XXX */
> +        put_OFPAT_SET_QUEUE(out, ofp_version, enqueue->queue);
> +
> +        struct ofp11_action_output *oao = put_OFPAT11_OUTPUT(out);
> +        oao->port = ofputil_port_to_ofp11(enqueue->port);
> +        oao->max_len = OVS_BE16_MAX;
> +
> +        put_NXAST_POP_QUEUE(out);
>     }
> }
> 
> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> index 23d3202..a3b4ccf 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -751,3 +751,20 @@ NXST_FLOW reply:
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([enqueue action for OF1.1+])
> +AT_KEYWORDS([ofp-actions])
> +OVS_VSWITCHD_START
> +dnl OpenFlow 1.0 has an "enqueue" action.  For OpenFlow 1.1+, we translate
> +dnl it to a series of actions that accomplish the same thing.
> +AT_CHECK([ovs-ofctl -O OpenFlow10 add-flow br0 'actions=enqueue(123,456)'])
> +AT_CHECK([ovs-ofctl -O OpenFlow10 dump-flows br0 | ofctl_strip], [0], [dnl
> +NXST_FLOW reply:
> + actions=enqueue:123:456
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip], [0], [dnl
> +OFPST_FLOW reply (OF1.3):
> + reset_counts actions=set_queue:456,output:123,pop_queue

I forget why we see 'reset_counts' here?

> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

On Thu, Jul 14, 2016 at 03:03:05AM -0700, Jarno Rajahalme wrote:
> With a small question below:
> 
> Acked-by: Jarno Rajahalme <jarno@ovn.org>

Thanks!

...

> > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-flows br0 | ofctl_strip], [0], [dnl
> > +OFPST_FLOW reply (OF1.3):
> > + reset_counts actions=set_queue:456,output:123,pop_queue
> 
> I forget why we see 'reset_counts' here?

It's dumb.

The OpenFlow specification implies that all flags supplies on flow_mod
should be preserved as part of the flow and returned on flow dumps.
reset_counts in particular doesn't make any sense there because it's not
a state, it's an action to take.  An OpenFlow 1.0 flow_mod always
implies reset_counts, so when we add a flow with OpenFlow 1.0 and then
dump them with a protocol that supports reset_counts (OF1.2+), we'll
always get reset_counts back.

The comments on enum ofputil_flow_mod_flags talk about this a little:

    /* These flags primarily affects flow_mod behavior.  They are not
     * particularly useful as part of flow state.  We include them in flow
     * state only because OpenFlow implies that they should be. */
    OFPUTIL_FF_CHECK_OVERLAP = 1 << 3, /* All versions. */
    OFPUTIL_FF_RESET_COUNTS  = 1 << 4, /* OpenFlow 1.2+. */