From owner-svn-src-all@FreeBSD.ORG Fri Jan 30 05:47:46 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CBC5372F; Fri, 30 Jan 2015 05:47:46 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 7807F365; Fri, 30 Jan 2015 05:47:46 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id D4D30786C4C; Fri, 30 Jan 2015 16:47:31 +1100 (AEDT) Date: Fri, 30 Jan 2015 16:47:26 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric Subject: Re: svn commit: r277898 - head/sys/fs/msdosfs In-Reply-To: <201501292030.t0TKUEG7021272@svn.freebsd.org> Message-ID: <20150130143552.M1034@besplex.bde.org> References: <201501292030.t0TKUEG7021272@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=baJSDo/B c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=WqrAmHMBs3C9wVo5yAoA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Jan 2015 05:47:46 -0000 On Thu, 29 Jan 2015, Dimitry Andric wrote: > Log: > Fix a bunch of -Wcast-qual warnings in msdosfs_conv.c, by using > __DECONST. No functional change. My flamethrower was not warm enough when __DECONST() was committed. I only removed (never merged) it in my version, and backed out a few early uses of it. > Modified: head/sys/fs/msdosfs/msdosfs_conv.c > ============================================================================== > --- head/sys/fs/msdosfs/msdosfs_conv.c Thu Jan 29 19:55:33 2015 (r277897) > +++ head/sys/fs/msdosfs/msdosfs_conv.c Thu Jan 29 20:30:13 2015 (r277898) > @@ -253,7 +253,7 @@ dos2unixfn(u_char dn[11], u_char *un, in > * Copy the name portion into the unix filename string. > */ > for (i = 8; i > 0 && *dn != ' ';) { > - c = dos2unixchr(tmpbuf, (const u_char **)&dn, &i, > + c = dos2unixchr(tmpbuf, __DECONST(const u_char **, &dn), &i, > lower & LCASE_BASE, pmp); > while (*c != '\0') { > *un++ = *c++; Fixing the API seems to be difficult, except removing its half-baked constification may be considered a fix. The problem is similar to the one for the execv() family but not the one for the strtol() family: the pointer is doubly indirect and has some constness, but neither 1 nor 2 const qualifiers on it works right. 2 const qualifiers for the exec() family would break automatic conversion from non-const data in all cases: int execv_2c(const char *path, char const * const *argv); ... char *myargs[16]; /* non-const ptrs to non-const data */ ... execv_2c(path, &myargs[0]); /* error (can't auto-add inner (first) const) */ Not being allowed to auto-add the inner const is a bug in C's type system. Allowing it would give even larger bugs. 1 (outer) const works for the naive uses of execv(): int execv(const char *path, char *const argv[]); /* actual decl */ ... char *myargs[16]; ... execv_2c(path, &myargs[0]); /* OK */ but this is almost useless. The prototype says that each pointer argv[i] is not modified but the string that it points to may be modified. So you can also have const pointers in myargs (char * const myargs[16]), but not const strings with either const or non-const pointers (char const * [const] myargs[16]). Zero consts also work for the naive use, but not when myargs has any consts. 1 (inner) breaks the naive use much like 2 inner consts: int execv_1inner(const char *path, char const **argv); ... char *myargs[16] ... execv_1inner(path, &myargs[0]); /* error (same as above) */ clang prints a bogus diagnostic about this: X z.c:15:7: warning: passing 'char **' to parameter of type 'const char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers] X execv_1inner(&myargs); X ^~~~~~~~~~ The bogusness is that it says that qualifiers are discarded, but they are actually added. This is related to the bug in the type system that would turn adding of qualifiers into discarding them if this addition were allowed. So the diagnostic may be technically correct, but it is confusing. execv_1inner() allows myargs to have const strings (char const *myargs[16]) without any bogus casts being needed. myargs still can't have const pointers. There are additional complications if &myargs[0] is spelled sloppily as &myargs. The consts prevent the latter being auto-converted to the former. dos2unixchr() is like execv_1inner(). Unlike execv(), it has the API that gives more type-safety but is harder to use on non-const strings. Like execv() but with less excuse, it doesn't use 2 consts, so the type safety is not complete. strtol() is quite different. It needs to modify the pointer, so it needs the _1inner variant to give maximum type safety. However, this would be too painful, so both consts are intentionally left out. Re-quoting: > - c = dos2unixchr(tmpbuf, (const u_char **)&dn, &i, > + c = dos2unixchr(tmpbuf, __DECONST(const u_char **, &dn), &i, The 1 const API tells callers that the string is not modified, but it is even more important that the pointer (dn here) is not modified. Correct fix (without changing the API): const char *cdn; ... cdn = dn; c = dos2unixchr(tmpbuf, &cdn), &i, This is painful, but is actually a couple of characters shorter (not counting whitespace). It doesn't matter if the pointer gets clobbered provided we re-initialize it before every call. > @@ -270,8 +270,8 @@ dos2unixfn(u_char dn[11], u_char *un, in > *un++ = '.'; > thislong++; > for (i = 3; i > 0 && *dn != ' ';) { > - c = dos2unixchr(tmpbuf, (const u_char **)&dn, &i, > - lower & LCASE_EXT, pmp); > + c = dos2unixchr(tmpbuf, __DECONST(const u_char **, &dn), > + &i, lower & LCASE_EXT, pmp); > while (*c != '\0') { > *un++ = *c++; > thislong++; msdosfs_conv.c also does many bogus conversions from char to u_char. It has many more bogus (const u_char **) casts than the ones changed to use __DECONST() in this commit, since the pointer is sometimes already const but not already for plain chars. By default, gcc[3.3 through 4.8] doesn't warn about sign mismatches in pointers for assignment or parameter passing, but clang does. So my fix needs more changes to preserve or fix the sign conversions. The API might be easier to fix here. Pathnames should be converted from chars to u_chars in only one place, possibly by copying them instead of using type puns. They really consist of u_chars (bytes) but consist of chars at the syscall level. Bruce