[PATCH] Make hash.h usable in the kernel

[Ruslan Ermilov]

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)
> >{
> >	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

> 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 **".

> 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:

: #include <stdlib.h>
: 
: static const char *s = "123a";
: 
: int
: main(void)
: {
:         long v;
:         char *endptr;
: 
:         endptr = NULL;
:         v = strtol(s, &endptr, 0);
:         if (endptr != NULL)
:                 *endptr = '\0';
:         return (0);
: }

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);
: }

Or differently: it's safe (and possible) to do "end1 = end2",
but not the opposite.

> Changing
> the prototype forces all callers to use "const char **end; ... &end",
                                                      ^ extra `*'

> and then if they want to modify *end, to convert `end' to plain char *.
> 
Not necessarily, see above.  And from the function's POV (whose prototype
we're considering), "end" will be made to point to a substring of a const
string, so obviously it will also point to a const string.

> Modifying *end is only valid if the original string is modifyable, and
> this case ends up needing lots of ugly casting away of const, which
> leads to compiler warnings, which lead to even uglier things like the
> __DECONST() mistake to "fix" the warnings.
> 
Not *lots* actually.  Passing "char *" where "const char *" is required
is safe and allowed, passing "char **" as "const char **" is allowed but
requires a (safe) cast.

> >@@ -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.)


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20061012/8f2bc85b/attachment.pgp