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

[Thomas Haller <thaller@redhat.com>]

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);


Maybe instead it should do:


void datatype_free(const struct datatype *dtype)
{
»·······if (!dtype)
»·······»·······return; 
»·······if (!(dtype->flags & DTYPE_F_ALLOC))
»·······»·······return;

»·······assert(dtype->refcnt != 0);

»·······if (--((struct datatype *)dtype)->refcnt > 0)
»·······»·······return;

»·······free_const(dtype->name);
»·······free_const(dtype->desc);
»·······free_const(dtype);
}

but the principle is still the same.

> 
> BTW: I found two lines in src/netlink.c reading:
> 
> > datatype_free(datatype_get(dtype));
> 
> Aren't those just fancy nops?

Indeed. Already fixed by
https://git.netfilter.org/nftables/commit/src/netlink.c?id=8519ab031d8022999603a69ee9f18e8cfb06645d




Thomas