[ovs-dev] [RFC v4 1/1] datapath: Add a new action check_pkt_len

[Numan Siddique]

On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192 at gmail.com> wrote:

>
>
> On 3/21/2019 5:38 PM, Gregory Rose wrote:
> >
> >
> > On 3/21/2019 10:37 AM, Numan Siddique wrote:
> >> This is the datapath patch -
> https://patchwork.ozlabs.org/patch/1046370/
> >> and this is the corresponding ovs-vswitchd patch -
> >> https://patchwork.ozlabs.org/patch/1059081/ (this is part of the
> >> series -
> >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=98190,
> >> but probably you would be interested in only ovs patch)
> >>
> >> Sharing the links so that you can find it easily.
> >>
> >> Thanks
> >> Numan
> >>
> >>
> >
> > This patch:
> >
> > https://patchwork.ozlabs.org/patch/1059081/
> >
> > shows this when applied:
> >
> > Applying: Add a new OVS action check_pkt_larger
> > .git/rebase-apply/patch:1097: new blank line at EOF.
> > +
> > warning: 1 line adds whitespace errors.
> >
> > In regards to the datapath patch 1046370
> > <https://patchwork.ozlabs.org/patch/1046370/>
> >
> > In execute_check_pkt_len():
> >
> > +
> > +       actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ?
> > VLAN_HLEN : 0);
> > +
> >
> > This doesn't seem right to me - the skb length should include the
> > length of the entire packet, including any
> > VLAN tags, or at least that is my understanding.  Please check it.
> >
> > In validate_and_copy_check_pkt_len() in flow_netlink.c:
> >
> > +       static const struct nla_policy pol[OVS_CHECK_PKT_LEN_ATTR_MAX
> > + 1] = {
> > +               [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NLA_U16 },
> > +               [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = {
> > +                       .type = NLA_NESTED },
> > +               [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = {
> > +                       .type = NLA_NESTED },
> > +       };
> >
> > I don't care for declaring these things within function scope and it
> > is not generally done.  I see that
> > flow_netlink.c has one other instance of the nla_policy structure
> > statically declared within the function scope
> > but if you look at datapath.c none of them are.  I prefer the way it's
> > done in datapath.c.  I also grepped around
> > in other kernel code in the ./net tree and that is also the way it's
> > done there, i.e. I didn't see any other
> > instances of it declared within function scope.
> >
> > I compiled both the ovs-vswitchd and openvswitch kernel module
> > components with no issues.  I wanted to use
> > clang but the version of clang on Ubuntu right now doesn't have
> > retpoline support so it won't compile
> > kernel modules.
> >
> > :-/
> >
> > I did some quick regression testing and found no problems.  If you can
> > address the two coding issues I brought up
> > then I'd be glad to add my reviewed and tested by tags.
>
> Oh wait, this is just an RFC.
>
> I'll review and test the patches again when they officially come out.
> Maybe clang will have retpoline support
> by then.
>
>
Thank you for the review. I will address them. I am planning to submit the
patch
to net-next ML. Looks like the window is open now -
http://vger.kernel.org/~davem/net-next.html

Please let me know in case you prefer to submit another RFC version here
before submitting to  net-dev ML.

Thanks
Numan

- Greg
>
>