On 3/24/2019 11:37 AM, Numan Siddique wrote:
On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <[email protected]
<mailto:[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] <mailto:[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
Thanks for clearing that up for me Numan.
- Greg
>
> 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
