Re: [PATCH v3 4/6] doc: add more documentation on bitmasks and sets

[Florian Westphal <fw@strlen.de>]

Christoph Anton Mitterer <mail@christoph.anton.mitterer.name> wrote:
> On Mon, 2025-10-20 at 11:06 +0200, Florian Westphal wrote:
> > Christoph Anton Mitterer <mail@christoph.anton.mitterer.name> wrote: 
> > > +Equality of a value with a set is given if the value matches
> > > exactly one value
> > > +in the set.
> > 
> > That contradicts whats right above, which describes range handling.
> 
> Uhm... what exactly do you mean?
>
> But that's anyway missing the stuff about how interval values are
> matched, which I only sent as stand alone patch in some mail before.

Yes, I meant wrt. intervals, there is no need for the value to be in
the set, match can also happen via range.

> > > +It shall be noted that for bitmask values this means, that
> > > +*'expression' 'bit'[,'bit']...* (which yields true if *any* of the
> > > bits are set)
> > > +is not the same as *'expression' {'bit'[,'bit']...}* (which yields
> > > true if
> > > +exactly one of the bits are set).
> > > +It may however be (effectively) the same, in cases like
> > > +`ct state established,related` and `ct state
> > > {established,related}`, where these
> > > +states are mutually exclusive.
> > 
> > Would you object if I apply this patch but onlt rhe first part?
> 
> You mean: not the changes to nft.txt?

Yes, only those in doc/data-types.txt, namely this:

diff --git a/doc/data-types.txt b/doc/data-types.txt
--- a/doc/data-types.txt
+++ b/doc/data-types.txt
@@ -26,6 +26,30 @@ integer
 
 The bitmask type (*bitmask*) is used for bitmasks.
 
+In expressions the bits of a bitmask may be specified as *'bit'[,'bit']...* with
+'bit' being the value of the bit or a pre-defined symbolic constant, if any (for
+example *ct state*’s bit 0x1 has the symbolic constant `new`).
+
+Equality of a value with such bitmask is given, if the value has any of the
+bitmask’s bits set (and optionally others).
+
+The syntax *'expression' 'value' / 'mask'* is identical to
+*'expression' and 'mask' == 'value'*.
+For example `tcp flags syn,ack / syn,ack,fin,rst` is the same as
+`tcp flags and (syn|ack|fin|rst) == syn|ack`.
+
+Note that *'expression' 'bit'[,'bit']...* is not the same as *'expression'
+{'bit'[,'bit']...}*.
+The latter form is a lookup in an anonymous set and will match only if the set
+contains a matching value.
+Example: *tcp flags syn,ack* matches packets that have the SYN, the ACK, or both
+SYN and ACK flags set.  Other flags, such as PSH, are ignored.
+*tcp flags { syn, ack }* matches packets that have only the SYN or only the ACK
+flag set, all other flag bits must be unset.
+
+As usual, the the *nft describe* command may be used to get details on a data
+type, which for bitmasks shows the symbolic names and values of the bits.

This contains minor edits only, I don't see anything wrong with the
above and I think that this is a worthwhile addition to the
documentation.

> What I think should be kept being added to the SETS documentation in
> nft.txt is:
> > Equality of a value with a set is given if the value matches exactly
> > one value
> > in the set (which for intervals means that it’s contained in any of
> > them).
> 
> Cause that's currently nowhere really documented, AFAICS.

Oh, yes, makes sense to mention that.

> The following:
> > It shall be noted that for bitmask values this means, that
> > *'expression' 'bit'[,'bit']...* (which yields true if *any* of the
> > bits are set)
> > is not the same as *'expression' {'bit'[,'bit']...}* (which yields
> > true if
> > exactly one of the bits are set).
> 
> Can be skipped, or maybe one should add a small reference like:
> > See <<BITMASK TYPE>> for how equality checks differ between sets and
> > bitmasks.

Reference is fine.

> This:
> > It may however be (effectively) the same, in cases like
> > `ct state established,related` and `ct state {established,related}`,
> > where these
> > states are mutually exclusive.
> 
> We can either simply drop, or move over to the BITMASK TYPE section.
> It's not super important, but I think there might be some value in
> understanding why these are identical (especially as many people use
> something like ct new {established,related}.

Yes, its an exception by virtue of a flow being either-or...

If you think it should be documented then maybe it should be added to
the bitmask example, it documents the difference with tcp flags example,
maybe the counterexample can be made there.

(Its not identical, however -- the established,related [ no set ] way is
slightly faster).

> Wanna a new patch or are you going to do it yourself?

Can you just revamp this one patch?  I thin its the one closest
to applicable form.