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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/07/2023 09:44, Juergen Gross wrote:
> On 26.07.23 10:20, Julien Grall wrote:
> > 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.
This is a pretty good example on how to not write an interface. :)

> > 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.
Well... I can always be swayed if there is a good argument to make it 
const. So far, you mention that hashtable doesn't modify the content but 
you don't really explain why waiving away the help from the compiler is 
ok. Therefore, to me it seems the downside is bigger than the benefit.
Also, I am not asking to drop the patch. The const on the key is ok. I 
am only requesting to remove the const on the value.
> 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.
Again, this still means we are casting away the const somewhere. This is 
the part I am against if there is no strong justification for it (i.e. 
there is no other way to do it).
Cheers,

--
Julien Grall