Re: [PATCH nft 3/4] all: add free_const() and use it instead of xfree()

[Phil Sutter <phil@nwl.cc>]

On Wed, Sep 20, 2023 at 09:48:40PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-20 at 20:22 +0200, Phil Sutter wrote:
> > On Wed, Sep 20, 2023 at 08:03:17PM +0200, Thomas Haller wrote:
> > > On Wed, 2023-09-20 at 18:49 +0200, Phil Sutter wrote:
> > > > On Wed, Sep 20, 2023 at 06:06:23PM +0200, Pablo Neira Ayuso
> > > > wrote:
> > > > > On Wed, Sep 20, 2023 at 04:13:43PM +0200, Phil Sutter wrote:
> > > > > > On Wed, Sep 20, 2023 at 03:13:40PM +0200, Thomas Haller
> > > > > > wrote:
> > > > > > [...]
> > > > > > > There are many places that rightly cast away const during
> > > > > > > free.
> > > > > > > But not
> > > > > > > all of them. Add a free_const() macro, which is like
> > > > > > > free(),
> > > > > > > but accepts
> > > > > > > const pointers. We should always make an intentional choice
> > > > > > > whether to
> > > > > > > use free() or free_const(). Having a free_const() macro
> > > > > > > makes
> > > > > > > this very
> > > > > > > common choice clearer, instead of adding a (void*) cast at
> > > > > > > many
> > > > > > > places.
> > > > > > 
> > > > > > I wonder whether pointers to allocated data should be const
> > > > > > in
> > > > > > the first
> > > > > > place. Maybe I miss the point here? Looking at flow offload
> > > > > > statement
> > > > > > for instance, should 'table_name' not be 'char *' instead of
> > > > > > using this
> > > > > > free_const() to free it?
> > > > > 
> > > > > The const here tells us that this string is set once and it
> > > > > gets
> > > > > never
> > > > > updated again, which provides useful information when reading
> > > > > the
> > > > > code IMO.
> > > > 
> > > > That seems like reasonable rationale. I like to declare function
> > > > arguments as const too in order to mark them as not being altered
> > > > by
> > > > the
> > > > function.
> > > > 
> > > > With strings, I find it odd to do:
> > > > 
> > > > const char *buf = strdup("foo");
> > > > free((void *)buf);
> > > > 
> > > > > I interpret from Phil's words that it would be better to
> > > > > consolidate
> > > > > this to have one single free call, in that direction, I agree.
> > > > 
> > > > No, I was just wondering why we have this need for free_const()
> > > > in
> > > > the
> > > > first place (i.e., why we declare pointers as const if we
> > > > allocate/free
> > > > them).
> > > 
> > > 
> > > I think that we use free_const() is correct.
> > > 
> > > 
> > > Look at "struct datatype", which are either immutable global
> > > instances,
> > > or heap allocated (and ref-counted). For the most part, we want to
> > > treat these instances (both constant and allocated) as immutable,
> > > and
> > > the "const" specifier expresses that well.
> > 
> > So why doesn't datatype_get() return a const pointer then?
> 
> Good point.
> 
> Also compare with 
> 
>   char *strchr(const char *s, int c);
> 
> where it makes sense.
> 
> For datatype_get() it makes less sense. I will send a patch.
> 
> 
> >  I don't find
> > struct datatype a particularly good example here: datatype_free()
> > does
> > not require free_const() at all.
> 
> datatype_free() in the patch uses+requires free_const() twice:
> 
> »·······free_const(dtype->name);
> »·······free_const(dtype->desc);
> »·······free(dtype);

Ah, I see. The only reason why these are allocated is
concat_type_alloc(), BTW. If it didn't exist, dtype_clone() could just
copy the pointers and datatype_free() would ignore them.

Cheers, Phil