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.
- Greg
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
