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

> /* Just free(), but casts to a (void*). This is for places where
>  * we have a const pointer that we know we want to free. We could just
>  * do the (void*) cast, but free_const() makes it clear that this is
>  * something we frequently need to do and it's intentional. */
> #define free_const(ptr) free((void *)(ptr))
> 
> I like this macro.
> 
> Maybe turn it into:
> 
>         nft_free(ptr)
> 
> and we use it everywhere?

I believe this is exactly what Thomas is trying to move away from. IIUC,
he wants to have a "special" free() to mark the spots where a const
pointer is freed (and make it a more deliberate action).

Cheers, Phil