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?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
