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.
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.
/* 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?
BTW, nitpick, netdev comment style is preferred:
/* 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.
*/
Thanks!
