| Message ID | 20230724110247.10520-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
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@suse.com> > --- > 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? Cheers,
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@suse.com> >> --- >> 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. Juergen
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@suse.com> >>> --- >>> 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,
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@suse.com> >>>> --- >>>> 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
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,
On 26.07.23 11:29, Julien Grall wrote: > 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). Okay, I'll drop the const attribute for the value parameter. Juergen
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; 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; if (h->flags & HASHTABLE_FREE_VALUE) talloc_steal(e, v); e->next = h->table[index]; diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h index 5a2cc4a4be..1da3af2648 100644 --- a/tools/xenstore/hashtable.h +++ b/tools/xenstore/hashtable.h @@ -48,8 +48,8 @@ create_hashtable(const void *ctx, const char *name, * If in doubt, remove before insert. */ -int -hashtable_add(struct hashtable *h, void *k, void *v); +int +hashtable_add(struct hashtable *h, const void *k, const void *v); /***************************************************************************** * hashtable_search
The key and value are never modified by hashtable code, so they should be marked as const. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - make value const, too. --- tools/xenstore/hashtable.c | 7 ++++--- tools/xenstore/hashtable.h | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-)