[ovs-dev] [RFC v3 2/5] Add a new OVS action check_pkt_larger
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.
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.
> 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" ?
I referred the existing action load and tried to use it the same way.
I will think of a better way.
> 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
> There isn't any documentation, but some is needed.
> Tests are needed.
I didn't work on these as I wanted to get feedback about the approach in
> A NEWS item is appropriate.
More information about the dev