Hi Ben, Few comments inline.
Thanks Numan On Wed, Feb 13, 2019 at 10:53 PM Numan Siddique <nusid...@redhat.com> wrote: > > 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. > > Unfortunately, in v4 it's still the same syntax. I will keep exploring. > 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 v4 of this patch, with little work, freezing seems to be happening properly. I have added the test case for this scenario as well. In ctx_cancel_freeze(), I had to set ctx->pause to NULL and after "do_xlate_actions()" returns for IF_GREATER scenario, had to set ctx->exit = false. I hope this is sufficient to have freezing work properly for both the paths. Thanks Numan > 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