On 1/16/23 07:21, Ales Musil wrote:
>
>
> On Fri, Jan 13, 2023 at 9:58 PM Ilya Maximets <i.maxim...@ovn.org
> <mailto:i.maxim...@ovn.org>> wrote:
>
> On 1/12/23 18:17, Ales Musil wrote:
> > Currently, the CT can be flushed by dpctl only by specifying
> > the whole 5-tuple. This is not very convenient when there are
> > only some fields known to the user of CT flush. Add new struct
> > ofputil_ct_match which represents the generic filtering that can
> > be done for CT flush. The match is done only on fields that are
> > non-zero with exception to the icmp fields.
> >
> > This allows the filtering just within dpctl, however
> > it is a preparation for OpenFlow extension.
> >
> > Reported-at: https://bugzilla.redhat.com/2120546
> <https://bugzilla.redhat.com/2120546>
> > Signed-off-by: Ales Musil <amu...@redhat.com <mailto:amu...@redhat.com>>
> > ---
> > v6: Rebase on top of current master.
> > Address comments from Ilya.
>
> Nit: this kind of version history is not really useful. It should say
> what actually changed. I don't remember every comment I made, other
> people have no idea what I was asking for.
>
> > v5: Add ack from Paolo.
> > v4: Fix a flush all scenario.
> > v3: Rebase on top of master.
> > Address the C99 comment and missing dpif_close call.
> > v2: Rebase on top of master.
> > Address comments from Paolo.
> > ---
> > NEWS | 3 +
> > include/openvswitch/ofp-util.h | 28 +++
> > lib/automake.mk <http://automake.mk> | 2 +
> > lib/ct-dpif.c | 210 ++++++++++-------------
> > lib/ct-dpif.h | 4 +-
> > lib/dpctl.c | 43 +++--
> > lib/dpctl.man | 15 +-
> > lib/ofp-ct-util.c | 303 +++++++++++++++++++++++++++++++++
> > lib/ofp-ct-util.h | 40 +++++
> > tests/system-traffic.at <http://system-traffic.at> | 102
> ++++++++++-
> > 10 files changed, 611 insertions(+), 139 deletions(-)
> > create mode 100644 lib/ofp-ct-util.c
> > create mode 100644 lib/ofp-ct-util.h
> >
<snip>
> > +bool
> > +ofputil_ct_tuple_is_zero(const struct ofputil_ct_tuple *tuple, uint8_t
> ip_proto)
> > +{
> > + bool is_zero = ipv6_is_zero(&tuple->src) &&
> ipv6_is_zero(&tuple->dst);
> > +
> > + if (!(ip_proto == IPPROTO_ICMP || ip_proto == IPPROTO_ICMPV6)) {
> > + is_zero = is_zero && !tuple->src_port && !tuple->dst_port;
> > + }
> > +
> > + return is_zero;
>
> Do we require ICMP tuples to always have at least one address specified?
> Or should the 'if' above have an 'else { return false; }' ?
>
>
> The ICMP is very problematic, if we do the 'else { return false; }' the
> shortcut for
> the full 5-tuple stops working. Without it if user specifies reply with only
> ICMP parameters
> we won't flush anything. I'm not sure what the correct approach here would be.
> Maybe documenting the limitation? Adding note that for ICMP in reply
> direction to work
> user needs to specify at least one IP?
I think, the correct solution to construct a mask of what the user
have specified while parsing and use the mask instead of values for
is_zero(). And also use it during comparison to con compare fields
that are not specified. Will that solve the problem?
Since partial matches are not supported, it can be a bitmask of
NXT_CT_TUPLE_* values.
It should be OK to just document for now. This also should be
mentioned in the header along with the definition of the struct
nx_ct_flush.
A bitmap thing can go as a fixup after the branching, if it works.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
