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
