Re: [PATCH nft 3/9] datatype: drop flags field from datatype

[Thomas Haller <thaller@redhat.com>]

On Thu, 2023-09-21 at 16:23 +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 20, 2023 at 09:23:46PM +0200, Thomas Haller wrote:
> > On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote:
> > > On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote:
> > > > Flags are not always bad. For example, as a function argument
> > > > they
> > > > allow
> > > > easier extension in the future. But with datatype's "flags"
> > > > argument and
> > > > enum datatype_flags there are no advantages of this approach.
> > > > 
> > > > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This
> > > > could
> > > > even
> > > >   be a bool:1 bitfield if we cared to represent the information
> > > > with
> > > >   one bit only. For now it's not done because that would not
> > > > help
> > > > reducing
> > > >   the size of the struct, so a bitfield is less preferable.
> > > > 
> > > > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent
> > > > static
> > > >   instances. Drop this redundant flag.
> > > 
> > > Not sure I want to rely on refcnt to zero to identify dynamic
> > > datatypes. I think we need to consolidate datatype_set() to be
> > > used
> > > not only where this deals with dynamic datatypes, it might help
> > > improve traceability of datatype assignment.
> > 
> > I don't understand. Could you elaborate about datatype_set()?
> 
> I wonder if we could use datatype_set() to attach static datatypes
> too, instead of manually attaching datatypes, such as:
> 
>         expr->dtype = &integer_type;
> 
> in case of future extensions, using consistently this helper function
> will help to identify datatype attachments.

I think `expr->dtype = &integer_type` is fine, if

- expr->dtype doesn't previously point to a datatype that requires
datatype_free() (e.g. because it's NULL).

- the new datatype requires no datatype_get() (e.g. because it's
static).

> 
> > Btw, for dynamically allocated instances the refcnt is always
> > positive,
> > and for static ones it's always zero. The DTYPE_F_ALLOC flag is
> > redundant.
> 
> That is a correct observation, but a (hipothetical) subtle bug in
> refcnt might lead to a dynamic datatype get to refcnt to zero, and
> that might be harder to track?
> 
> Let me have a look if I can come up with some counter proposal to get
> rid of this flag, I would prefer not to infer the datatype class from
> reference counter value.

If the reference counting is messed up, there is either a leak, a use-
after-free or modification of static data. These are all bad bugs that
needs fixing and are avoided by best practices and testing.

IMO keeping redundant state does not help with that or with
readability.



I'd like to replace the "unsigned int flags" field with individual
boolean fields like "bool f_prefix" or "bool f_alloc".

Dropping DTYPE_F_ALLOC/f_alloc flag altogether can be done (or not
done) independently from that.


Thomas