Re: [PATCH v3 08/25] tools/xenstore: make hashtable key and value parameters const

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 26/07/2023 07:19, Juergen Gross wrote:
> On 25.07.23 18:08, Julien Grall wrote:
> > Hi,
> >
> > On 24/07/2023 12:02, Juergen Gross wrote:
> > > The key and value are never modified by hashtable code, so they should
> > > be marked as const.
> > You wrote this but...
> >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > > V3:
> > > - make value const, too.
> > > ---
> > >   tools/xenstore/hashtable.c | 7 ++++---
> > >   tools/xenstore/hashtable.h | 4 ++--
> > >   2 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
> > > index 11f6bf8f15..670dc01003 100644
> > > --- a/tools/xenstore/hashtable.c
> > > +++ b/tools/xenstore/hashtable.c
> > > @@ -11,7 +11,8 @@
> > >   struct entry
> > >   {
> > > -    void *k, *v;
> > > +    const void *k;
> > > +    void *v;
> > ... this is not const and ...
> >
> > >       unsigned int h;
> > >       struct entry *next;
> > >   };
> > > @@ -140,7 +141,7 @@ static int hashtable_expand(struct hashtable *h)
> > >       return 0;
> > >   }
> > > -int hashtable_add(struct hashtable *h, void *k, void *v)
> > > +int hashtable_add(struct hashtable *h, const void *k, const void *v)
> > >   {
> > >       /* This method allows duplicate keys - but they shouldn't be 
> > > used */
> > >       unsigned int index;
> > > @@ -164,7 +165,7 @@ int hashtable_add(struct hashtable *h, void *k, 
> > > void *v)
> > >       e->k = k;
> > >       if (h->flags & HASHTABLE_FREE_KEY)
> > >           talloc_steal(e, k);
> > > -    e->v = v;
> > > +    e->v = (void *)v;
> > ... you cast-away the const here. I think this is a pretty bad idea.
> >
> > Can you clarify why you are doing like that?
> The value is never changed by the hashtable code, but it might be 
> changed by
> e.g. a caller of hashtable_search() (see e.g. callers of 
> find_domain_struct()).
> Somewhere I need to cast the const away. I could do so in 
> hashtable_search()
> in case you prefer me to do so.
My problem is not with the placement of the const but the fact you are 
removing the const.
I agree that the hashtable code is not meant to modify the content. 
However, as you wrote, the caller of hashtable_search() could modify the 
content. So, for me, the value should not be const in the hashtable code.
To give a concrete example, with the current interface we are telling 
the user that what they store in the hashtable can be modified at some 
point. By adding 'const' for the value in hashtable_add(), we can 
mislead a user to think it is fine to store static string, yet this is 
not enforced all the way through. So one could mistakenly think that 
values returned hashtable_search() can be modified. And the compiler 
will not be here to help enforcing it because you cast-away the const.
Do you have any code in this series that requires the 'const' in 
hashtable_add()? If so, can you point me to the patch and I will have a 
look?
If not, then I will strongly argue that this should be dropped because 
dropping a const is always a recipe for disaster.
Cheers,

--
Julien Grall