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

[Juergen Gross <jgross@xxxxxxxx>]

On 26.07.23 10:20, Julien Grall wrote:
> 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.
This is arguable IMHO.

> 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.
Yes, like in the case of strstr().

It takes two const char * parameters and it is returning char *, even with it
pointing into the first parameter.

> 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?
I had it when writing this patch, but this requirement is gone now. But please
note that this means to drop the const from db_write(), too.

> If not, then I will strongly argue that this should be dropped because dropping 
> a const is always a recipe for disaster.
Depends IMO.

I believe it is better as I've done it, but in case you insist on it I can drop
the patch.

An alternative would be make hashtable_search() return a const and only cast the
const away where it is really needed (and probably with a prominent comment at
the related hashtable_add() place). I think this will hit xenstored_domain.c use
cases only.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature