On 20 Nov 2020, at 22:12, Jakub Kicinski wrote:
On Thu, 19 Nov 2020 04:04:04 -0500 Eelco Chaudron wrote:
Currently, the openvswitch module is not accepting the correctly
formated
netlink message for the TTL decrement action. For both setting and
getting
the dec_ttl action, the actions should be nested in the
OVS_DEC_TTL_ATTR_ACTION attribute as mentioned in the openvswitch.h
uapi.
IOW this change will not break any known user space, correct?
It will not as there isn’t any yet. Unfortunately, the original patch
was sent out without a userspace part. It was internally tested by the
original authors and not properly reviewed to bring forward the issue.
They did add some weird code to work around it.
But existing OvS user space already expects it to work like you
make it work now?
What's the harm in leaving it as is?
Without this change, the different Datapaths in OVS behave differently,
making the code to be datapath agnostic having to do all kinds of weird
tricks to work around it.
But even worse, the patch in the current format could interpret
additional options/attributes as actions, due to the actions not being
encapsulated/nested within the actual attribute.
Fixes: 744676e77720 ("openvswitch: add TTL decrement action")
Signed-off-by: Eelco Chaudron <echau...@redhat.com>
Can we get a review from OvS folks? Matteo looks good to you (as the
original author)?
See Matteo’s reply, looks like he is ok with this change.
- err = __ovs_nla_copy_actions(net, attr, key, sfa, eth_type,
+ err = __ovs_nla_copy_actions(net, actions, key, sfa, eth_type,
vlan_tci, mpls_label_count, log);
if (err)
return err;
You're not canceling any nests on error, I assume this is normal.
Yes, on error the sfa actions are not used.
+ add_nested_action_end(*sfa, action_start);
add_nested_action_end(*sfa, start);
return 0;
}
