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

Except, we still want to use ref/unref operations (which are called
datatype_get()/datatype_free()). Those operate on "const struct
datatype *", otherwise they would require a cast all the time (which is
cumbersome and on the contrary decreases type-safety).

It also means, the "refcnt" field of a "const struct datatype *" gets
mutated by ref/unref, and that's correct. See also, C++'s "mutable"
type qualifiers.

The free_const() usage is a consequence of that, and in many cases
correct. There might be places where we wrongly treat mutable data via
const-pointers. Those should be fixed. See "[PATCH nft 1/4] datatype:
don't return a const string from cgroupv2_get_path()" for an example.


Thomas