Date: Fri, 13 Oct 2006 21:46:20 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Ruslan Ermilov <ru@freebsd.org> Cc: freebsd-net@freebsd.org, andre@freebsd.org Subject: Re: [PATCH] Make hash.h usable in the kernel Message-ID: <20061013204457.G50563@delplex.bde.org> In-Reply-To: <20061012072036.GA60767@rambler-co.ru> References: <20061011090241.GA2831@FreeBSD.czest.pl> <20061011094049.GA24964@rambler-co.ru> <20061012052101.A814@epsplex.bde.org> <20061012072036.GA60767@rambler-co.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 12 Oct 2006, Ruslan Ermilov wrote:
> On Thu, Oct 12, 2006 at 05:21:09AM +1000, Bruce Evans wrote:
>> On Wed, 11 Oct 2006, Ruslan Ermilov wrote:
>>> %%%
>>> Index: sys/sys/hash.h
>>> ===================================================================
>>> RCS file: /home/ncvs/src/sys/sys/hash.h,v
>>> retrieving revision 1.2
>>> diff -u -p -r1.2 hash.h
>>> --- sys/sys/hash.h 12 Mar 2006 15:34:33 -0000 1.2
>>> +++ sys/sys/hash.h 11 Oct 2006 09:38:50 -0000
>>> @@ -86,7 +86,7 @@ hash32_strn(const void *buf, size_t len,
>>> * namei() hashing of path name parts.
>>> */
>>> static __inline uint32_t
>>> -hash32_stre(const void *buf, int end, char **ep, uint32_t hash)
>>> +hash32_stre(const void *buf, int end, const char **ep, uint32_t hash)
BTW, the man page is missing the first `const' for all the functions.
>>> {
>>> const unsigned char *p = buf;
>>>
>>
>> I think this would break passing ep in almost all callers,
>>
> There are no callers of these functions yet, at least not in the
> current FreeBSD kernel. There are only 2 callers in OpenBSD,
> both in sys/kern/vfs_lookup.c
Oops, I thought this API was being adapted from userland.
>> in the same
>> way that "fixing" the corresponding arg in the strtol(3) family would
>> break almost all callers.
>>
> Yes, but strtol(3) has seen more life in sin. ;)
>
>> Callers want and need to pass char **, but
>> char ** is not compatible with const char **.
>>
> Not compatible, but "char **" can safely be casted to "const char **".
No, this is unsafe. See C99 6.5.16.1 [#6] (at least in the n869.txt draft).
C99 requires a diagnostic for the corresponding assignment, but not of
course for the unsafe cast.
>> Callers want to do this
>> because it's easier to write "char *end; ... &end", and they often
>> need to do this so that they can modify the resulting *end.
>>
> But this is bad practice; if string is really const, writing to *end
> will SIGBUS, and the fact that interface has it spelled as "char **"
> doesn't mitigate it:
Often (perhaps usually for strtol()), the data isn't const...
> OTOH, if string is really modifiable, then simple casting when calling
> a function works:
>
> : #include <stdlib.h>
> :
> : void foo(const char *, char *);
> : void bar(const char *, const char **);
> :
> : void
> : foo(const char *s1, char *s2)
> : {
> : const char *end1 = NULL;
> : char *end2 = NULL;
> :
> : bar(s1, &end1);
> : bar(s2, (const char **)&end2);
> : }
Hmm, gcc -Wcast-qual doesn't warn about this cast. I think it should,
since allowing this is equivalent to allowing casting away of const.
Maybe gcc is allowing a loophole or just making no more effort than the
C standard to make const work right at all levels.
> Or differently: it's safe (and possible) to do "end1 = end2",
> but not the opposite.
That's different -- there is no ** in sight and both gcc and the C
standard get things right.
>>> @@ -94,7 +94,7 @@ hash32_stre(const void *buf, int end, ch
>>> hash = HASHSTEP(hash, *p++);
>>>
>>> if (ep)
>>> - *ep = (char *)p;
>>> + *ep = (const char *)p;
>>>
>>> return hash;
>>> }
>>
>> Doesn't this cause a cast-qual warning in the kernel?
>>
> Why? None of qualifiers are lost as a result of cast; both "p" and "ep"
> are pointers to const-qualified base types. (No, it doesn't cause a
> warning.)
Oops, I missed that it is the old cast that is unsafe. Now I think it
was only the warning for that cast which made hash.h unusable (the original
mail said why, but I forget the details).
The new cast is of course safe but is still weird. The old cast was
mainly to break the diagnostic (fatal in gcc?) about the type mismatch
due to assigning away `const'. With recent or newer versions of gcc,
it would also break the diagnostic (warning in gcc) about the type
mismatch due to different signedness. But why are we returning "[const]
char *"? We start with "const void *" and use it as "const unsigned
char *". Then we switch signedness and indirectly return "[const]
char *". Why not indirectly return "[const] unsigned char *", or
better "[const] void *"?
Other problems in hash.h:
- missing `restrict' ?
- namespace problems limit it to the kernel
- minor style bugs.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061013204457.G50563>