From owner-freebsd-net@FreeBSD.ORG Fri Oct 13 11:47:22 2006 Return-Path: X-Original-To: freebsd-net@freebsd.org Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D687B16A40F; Fri, 13 Oct 2006 11:47:22 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1-3.pacific.net.au [61.8.2.210]) by mx1.FreeBSD.org (Postfix) with ESMTP id E32AE43E3B; Fri, 13 Oct 2006 11:46:40 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.2.163]) by mailout1.pacific.net.au (Postfix) with ESMTP id BDD0A64D2FE; Fri, 13 Oct 2006 21:46:22 +1000 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (Postfix) with ESMTP id 495D32742D; Fri, 13 Oct 2006 21:46:21 +1000 (EST) Date: Fri, 13 Oct 2006 21:46:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Ruslan Ermilov In-Reply-To: <20061012072036.GA60767@rambler-co.ru> Message-ID: <20061013204457.G50563@delplex.bde.org> References: <20061011090241.GA2831@FreeBSD.czest.pl> <20061011094049.GA24964@rambler-co.ru> <20061012052101.A814@epsplex.bde.org> <20061012072036.GA60767@rambler-co.ru> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-net@freebsd.org, andre@freebsd.org Subject: Re: [PATCH] Make hash.h usable in the kernel X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Oct 2006 11:47:23 -0000 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 > : > : 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