On 12/4/23 06:11, Ales Musil wrote: > > > On Sat, Dec 2, 2023 at 12:21 AM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@ovn.org>> wrote: > > On 11/30/23 08:31, Ales Musil wrote: > > CT flush extension would silently ignore unknown properties, > > which could lead to potential surprise by deleting more than > > it was requested to. Return error on unknown property instead > > to avoid this problem and at the same time inform the user > > that the specified property is not supported. > > > > Fixes: 08146bf7d9b4 ("openflow: Add extension to flush CT by generic > match.") > > Signed-off-by: Ales Musil <amu...@redhat.com <mailto:amu...@redhat.com>> > > --- > > v2: Rebase on top of current master. > > Address comments from Ilya: > > - Add the check also into ofp_ct_tuple_decode_nested. > > --- > > lib/ofp-ct.c | 11 +++++++++++ > > tests/ofp-print.at <http://ofp-print.at> | 18 ++++++++++++++++++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > > index 85a9d8bec..53964cf09 100644 > > --- a/lib/ofp-ct.c > > +++ b/lib/ofp-ct.c > > @@ -31,6 +31,9 @@ > > #include "openvswitch/ofp-prop.h" > > #include "openvswitch/ofp-util.h" > > #include "openvswitch/packets.h" > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(ofp_ct); > > > > static void > > ofp_ct_tuple_format(struct ds *ds, const struct ofp_ct_tuple *tuple, > > @@ -286,6 +289,10 @@ ofp_ct_tuple_decode_nested(struct ofpbuf > *property, struct ofp_ct_tuple *tuple, > > case NXT_CT_TUPLE_ICMP_CODE: > > error = ofpprop_parse_u8(&inner, &tuple->icmp_code); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_tuple", type); > > + break; > > } > > > > if (error) { > > @@ -377,6 +384,10 @@ ofp_ct_match_decode(struct ofp_ct_match *match, > bool *with_zone, > > } > > error = ofpprop_parse_u16(&property, zone_id); > > break; > > + > > + default: > > + error = OFPPROP_UNKNOWN(false, "ofp_ct_match", type); > > + break; > > } > > > > if (error) { > > diff --git a/tests/ofp-print.at <http://ofp-print.at> > b/tests/ofp-print.at <http://ofp-print.at> > > index 14aa55416..b4b4c6685 100644 > > --- a/tests/ofp-print.at <http://ofp-print.at> > > +++ b/tests/ofp-print.at <http://ofp-print.at> > > @@ -4180,4 +4180,22 @@ AT_CHECK([ovs-ofctl ofp-print "\ > > 00 01 00 20 00 00 00 00 \ > > 00 00 00 14 00 00 00 00 00 00 00 00 00 00 ff ff 0a 0a 00 02 00 00 00 > 00 \ > > " | grep -q OFPBPC_BAD_VALUE], [0]) > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 20 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 80 00 08 00 00 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_match property type 128" stderr], > [0]) > > Hmm, the 'ofp_ct_match' is not really something user-visible. > I think, it's better to point to the actual OpenFlow entity > here, i.e. 'NXT_CT_FLUSH'. > > > + > > +AT_CHECK([ovs-ofctl ofp-print "\ > > +01 04 00 28 00 00 00 03 00 00 23 20 00 00 00 20 \ > > +06 \ > > +00 00 00 00 00 00 00 \ > > +00 00 00 10 00 00 00 00 \ > > +00 80 00 08 00 50 00 00 \ > > +"| grep -q OFPBPC_BAD_TYPE], [0], [ignore], [stderr]) > > +AT_CHECK([grep -q "unknown ofp_ct_tuple property type 128" stderr], > [0]) > > Same here. We do not have a direct replacement, but something like > 'NXT_CT_TUPLE' might make sense as it is a prefix of the actual > properties here. > > If you agree, I can replace these while applying the patch. > > WDYT? > > > That sounds reasonable.
Thanks! I made the change. Applied and backported down to 3.1. Best regards, Ilya Maximets. > > > > Best regards, Ilya Maximets. > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amu...@redhat.com <mailto:amu...@redhat.com> > > <https://red.ht/sig> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
