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 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? I don't find
struct datatype a particularly good example here: datatype_free() does
not require free_const() at all.

BTW: I found two lines in src/netlink.c reading:

| datatype_free(datatype_get(dtype));

Aren't those just fancy nops?

Cheers, Phil