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