svn commit: r277898 - head/sys/fs/msdosfs

[Bruce Evans]

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