[ovs-dev] [RFC v3 2/5] Add a new OVS action check_pkt_larger

[Ben Pfaff]

On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusiddiq at redhat.com wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> This patch adds a new action 'check_pkt_larger' which checks if the
> packet is larger than the given size and stores the result in the
> destination register.

Thanks.

This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the
"userspace only" group of datapath actions.  It should not do that.
It should also not remove OVS_CLONE_ATTR_EXEC.

ofpact_remaining_len() should not return bool.  (This mistake suggests
that the case where it should return nonzero was not tested.)

I don't understand the check for check_pkt_larger->dst.field in
xlate_check_pkt_larger().  The action would be invalid if this was
missing.  Such an ofpact should not be translated.

The following comment appears incomplete:
        /* If datapath doesn't support check_pkt_len action, then set the
         * SLOW_ACTION flag so that ..
         */

odp-util.c has code for formatting the new action, but not for parsing.
We need both.

The format might be a little too cute.  It might be wise to use
something more like check_pkt_len(size=123, gt(...), le(...)).

decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field
specified and the bits in it are valid and usable and return an error if
they are not.

As is, the OpenFlow action format only allows NXM headers.  All new
actions should also support OXM 64-bit experimenter headers.  Look
around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header()
for more information.

The OpenFlow syntax looks really weird.  I don't understand it at all.

I doubt that the action translation handles the case where freezing has
to happen properly.  I don't think we have any other actions where
there are two opportunities to freeze translation.  This might require
novel work.

In check_check_pkt_len(), the inner ct_clear action kind of mixes up two
different checks.  I don't think that check_pkt_len should rely on
ct_clear.  I recommend putting some other action inside, if one is
needed.

There isn't any documentation, but some is needed.

Tests are needed.

A NEWS item is appropriate.

Thanks,

Ben.