On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <[email protected]> wrote:
> > On 3/22/2019 2:58 AM, Numan Siddique wrote: > > > > On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <[email protected]> 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. >> > Hi Greg, I checked and tested it it. I can confirm that skb->len doesn't include the vlan header. Existing code in flow.c also uses the same way to get the packet len - https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 Thanks Numan > >> > 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. > > > I think you're good to go. > > Thanks, > > - Greg > > > Thanks > Numan > > - Greg >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
