On Mon, Jan 16, 2023 at 10:53 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
> 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. > > Yes, that would work and it would be probably faster. I will update the NXT_CT_TUPLE_* values, so they can be included in bitmask without breaking compatibility with the current approach. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
