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

Numan Siddique nusiddiq at redhat.com
Wed Feb 13 17:23:53 UTC 2019


Thanks Ben for providing the review comments.

I will address all the comments.

A couple of comments inline.

Thanks
Numan


On Tue, Feb 12, 2019 at 8:33 AM Ben Pfaff <blp at ovn.org> wrote:

> 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.
>

By OpenFlow syntax you mean the way this action is being used ?

i.e  "check_pkt_larger(1500)->NXM_NX_REG0[0]" ?

I  referred the existing action load and tried to use it the same way.
I will think of a better way.

Thanks


set

>
> 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.
>

Ack.
I didn't work on these as I wanted to get feedback about the approach in
general.

Thanks
Numan



> A NEWS item is appropriate.
>
> Thanks,
>
> Ben.
>


More information about the dev mailing list