Re: [PATCH] Make hash.h usable in the kernel

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
_______________________________________________
freebsd-net@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscribe@xxxxxxxxxxx"

Relevant Pages