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 <b...@ovn.org> wrote: > On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusid...@redhat.com wrote: > > From: Numan Siddique <nusid...@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. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
