On 2/11/2019 6:27 PM, Ben Pfaff wrote:
Greg, I recommend that you take a look at this one.
My apologies but I haven't been feeling well the last few days. I'll
have a look at this one.
Thanks,
(More commentary below.)
On Thu, Jan 10, 2019 at 11:29:48PM +0530, [email protected] wrote:
From: Numan Siddique <[email protected]>
[Please note, this patch is submitted as RFC in ovs-dev ML to
get feedback before submitting to netdev ML]
This patch adds a new action which checks the packet length and
executes a set of actions if the packet length is greater than
the specified length or executes another set of actions if the
packet length is lesser or equal to.
This action takes below nlattrs
* OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
to apply if the packet length is greater than the specified 'pkt_len'
* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
actions to apply if the packet length is lesser or equal to the
specified 'pkt_len'.
The main use case for adding this action is to solve the packet
drops because of MTU mismatch in OVN virtual networking solution.
When a VM (which belongs to a logical switch of OVN) sends a packet
destined to go via the gateway router and if the nic which provides
external connectivity, has a lesser MTU, OVS drops the packet
if the packet length is greater than this MTU.
With the help of this action, OVN will check the packet length
and if it is greater than the MTU size, it will generate an
ICMP packet (type 3, code 4) and includes the next hop mtu in it
so that the sender can fragment the packets.
I'm not used to seeing series where different patches apply to different
repos! It took me a while to figure out why "git am" didn't want to
work at all (but then I actually looked at the patch).
I didn't have a net-next tree handy so my comments are based on reading
the patch without applying or building it.
There is a special issue with this action, which is a lot like a special
issue with the "sample". That is that the action has flow control that
might change the flow in different ways, and this can make an important
difference for actions that follow this one. If one fork of the action
pops off a header, and the other one does not, then that makes
validating actions that come after it complicated. I do not see
anything here that takes that into account. I recommend reading the
validation code for the sample action for inspiration.
In validate_and_copy_check_pkt_len(), it might be better to use
nla_policy and nla_parse_nested(). Or maybe not. I did not look
closely.
I don't think that execute_check_pkt_len() should need to check for
netlink format errors. The validation code should take care of that.
It might also be able to assume a particular order for the attributes,
depending on how you implement validation.
I'm still not thrilled by the naming. I don't have any wonderful names
though.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
