On 11 November 2015 at 14:00, Jarno Rajahalme <ja...@ovn.org> wrote: > >> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestrin...@nicira.com> wrote: >> >> There are currently a few holes in how OVS verifies connection tracking >> fields >> and actions, pointed out by Ravindra Kenchappa. This series aims to verify >> ct_state,ct_zone,ct_mark,ct_label match fields and the ct() action more >> strictly. >> >> Patches 1-2 are straight up fixes for the field verification. Patches 3-6 >> have >> been changed a bit based on v1 feedback, although I'm not entirely sure if >> they're the right approach, so I welcome further feedback. >> >> In v2, the 'ofproto' is extended to have an additional function on the >> ofproto >> for action verification which is separate from the verification done in >> rule_construct(). This is mainly proposed to avoid introducing another loop >> across rule actions during rule_construct(). Prior to rule_construct(), >> ofproto_check_ofpacts() loops across to check that groups are valid. This >> series adds a function to the ofproto which will be called from this point >> for >> every action in each flowmod that is processed, allowing ofproto >> implementations to reject specific actions based on ofproto-specific >> criteria, >> for instance ofproto-dpif supports underlying datapaths that may not support >> connection tracking. I would appreciate feedback specifically on whether this >> error checking is worth splitting out from the rule_construct() phase. My >> current inclination is that it increases the complexity of the ofproto rule >> construction lifecycle, and avoiding an additional iteration would not >> actually >> provide any benefit as the new function must be chased through a class for >> every action, even if the implementation does not care about checking the >> majority of action types. Regardless, I have included the patch so we can >> review how this change looks. >> > > A high level comment to this point: ofproto_check_ofpacts() is already > checking only specific actions, the rest being (mostly correctly) assumed to > be supported. So, in this series I would expect that the new ofproto provider > call would only be made for actions that are known to not always be > supported, i.e., for the CT action only for now.
Thanks for the review, I have pushed patches 1-3 to master. I'll resend the actions checking patches following the feedback. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
