On Sat, Dec 2, 2023 at 12:21 AM Ilya Maximets <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>
> > ---
> > 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 | 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 b/tests/ofp-print.at
> > index 14aa55416..b4b4c6685 100644
> > --- a/tests/ofp-print.at
> > +++ b/tests/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.
>
> Best regards, Ilya Maximets.
>
> Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
