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. > > > > 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
