| Message ID | 20231128074622.42661-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,1/2] ofp-ct: Return error for unknown property in CT flush. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 11/28/23 08:46, 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 <amusil@redhat.com> > --- > lib/ofp-ct.c | 7 +++++++ > tests/ofp-print.at | 9 +++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > index 85a9d8bec..c4fabbe84 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, > @@ -377,6 +380,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; Hi, Ales. There is a similar check missing in ofp_ct_tuple_decode_nested(). Could you please add it as well? Best regards, Ilya Maximets.
On Wed, Nov 29, 2023 at 3:58 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 11/28/23 08:46, 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 <amusil@redhat.com> > > --- > > lib/ofp-ct.c | 7 +++++++ > > tests/ofp-print.at | 9 +++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c > > index 85a9d8bec..c4fabbe84 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, > > @@ -377,6 +380,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; > > Hi, Ales. > > There is a similar check missing in ofp_ct_tuple_decode_nested(). > Could you please add it as well? > > Best regards, Ilya Maximets. > > Added in v2. Thanks, Ales
Ales Musil <amusil@redhat.com> writes: > 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 <amusil@redhat.com> > --- This is a test - please don't change the state in patchwork yet. Recheck-request: github
diff --git a/lib/ofp-ct.c b/lib/ofp-ct.c index 85a9d8bec..c4fabbe84 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, @@ -377,6 +380,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..b370280b7 100644 --- a/tests/ofp-print.at +++ b/tests/ofp-print.at @@ -4180,4 +4180,13 @@ 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]) + AT_CLEANUP
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 <amusil@redhat.com> --- lib/ofp-ct.c | 7 +++++++ tests/ofp-print.at | 9 +++++++++ 2 files changed, 16 insertions(+)