Date: Tue, 24 Feb 2009 17:49:47 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r188934 - head/tools/regression/fstest Message-ID: <20090224172129.W10644@delplex.bde.org> In-Reply-To: <200902230733.n1N7XTUB017956@svn.freebsd.org> References: <200902230733.n1N7XTUB017956@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 23 Feb 2009, Pawel Jakub Dawidek wrote:
> Log:
> Add explicit casting in few places.
Most of these casts are excessive.
> It is only really necessary for open(2)'s third argument, which is optional and
> obtained through stdarg(3).
This arg is variadic so a cast is needed. For all others, a prototype should
be in scope so the casts have no effect except possibly to cause different
warnings.
> open(2)'s third argument is 32bit and we pass 64
> bits. On little endian it works, because we take lower 32 bits, but on big
Actually, open()'s 3rd arg is the unary promotion of mode_t. This just
happens to be 32 bits on all supported machines.
> Modified: head/tools/regression/fstest/fstest.c
> ==============================================================================
> --- head/tools/regression/fstest/fstest.c Mon Feb 23 06:47:10 2009 (r188933)
> +++ head/tools/regression/fstest/fstest.c Mon Feb 23 07:33:29 2009 (r188934)
> @@ -337,7 +337,7 @@ show_stat(struct stat64 *sp, const char
> printf("%lld", (long long)sp->st_ctime);
> #ifdef HAS_CHFLAGS
> else if (strcmp(what, "flags") == 0)
> - printf("%s", flags2str(chflags_flags, sp->st_flags));
> + printf("%s", flags2str(chflags_flags, (long long)sp->st_flags));
> #endif
Here the cast does nothing except possibly break warnings that flags2str()
has a bogus type. st_flags has type fflags_t which happens to be unsigned
32 bits, while the function to print it takes a larger type with different
signedness. Since the arg type happens to be larger on all supported
machines, there are no problems with sign extension bugs.
> else if (strcmp(what, "type") == 0) {
> switch (sp->st_mode & S_IFMT) {
> @@ -443,17 +443,17 @@ call_syscall(struct syscall_desc *scall,
> fprintf(stderr, "too few arguments\n");
> exit(1);
> }
> - rval = open(STR(0), flags, (mode_t)NUM(2));
> + rval = open(STR(0), (int)flags, (mode_t)NUM(2));
> } else {
> if (i == 3) {
> fprintf(stderr, "too many arguments\n");
> exit(1);
> }
> - rval = open(STR(0), flags);
> + rval = open(STR(0), (int)flags);
> }
> break;
> case ACTION_CREATE:
> - rval = open(STR(0), O_CREAT | O_EXCL, NUM(1));
> + rval = open(STR(0), O_CREAT | O_EXCL, (mode_t)NUM(1));
> if (rval >= 0)
> close(rval);
> break;
THe casts for open() are needed, and you used the correct one (not to
int32_t like the log message says).
> @@ -461,7 +461,7 @@ call_syscall(struct syscall_desc *scall,
> rval = unlink(STR(0));
> break;
> case ACTION_MKDIR:
> - rval = mkdir(STR(0), NUM(1));
> + rval = mkdir(STR(0), (mode_t)NUM(1));
> break;
> case ACTION_RMDIR:
> rval = rmdir(STR(0));
> @@ -476,30 +476,30 @@ call_syscall(struct syscall_desc *scall,
> rval = rename(STR(0), STR(1));
> break;
> case ACTION_MKFIFO:
> - rval = mkfifo(STR(0), NUM(1));
> + rval = mkfifo(STR(0), (mode_t)NUM(1));
> break;
> case ACTION_CHMOD:
> - rval = chmod(STR(0), NUM(1));
> + rval = chmod(STR(0), (mode_t)NUM(1));
> break;
> #ifdef HAS_LCHMOD
> case ACTION_LCHMOD:
> - rval = lchmod(STR(0), NUM(1));
> + rval = lchmod(STR(0), (mode_t)NUM(1));
> break;
All the above functions should have a prototype in scope, so no casts
are needed for the mode_t args. Casts would be needed to support K&R,
but I bet this code isn't closed to working with a K&R compiler since
the necessary casts are still missing in most places. Please add them
in all places or none :-).
> #endif
> case ACTION_CHOWN:
> - rval = chown(STR(0), NUM(1), NUM(2));
> + rval = chown(STR(0), (uid_t)NUM(1), (gid_t)NUM(2));
> break;
> case ACTION_LCHOWN:
> - rval = lchown(STR(0), NUM(1), NUM(2));
> + rval = lchown(STR(0), (uid_t)NUM(1), (gid_t)NUM(2));
> break;
Here the casts have no effect since a prototype is in scope.
> #ifdef HAS_CHFLAGS
> case ACTION_CHFLAGS:
> - rval = chflags(STR(0), str2flags(chflags_flags, STR(1)));
> + rval = chflags(STR(0), (unsigned long)str2flags(chflags_flags, STR(1)));
This excessive cast also causes a line longer than 80 characters.
> break;
> #endif
> #ifdef HAS_LCHFLAGS
> case ACTION_LCHFLAGS:
> - rval = lchflags(STR(0), str2flags(chflags_flags, STR(1)));
> + rval = lchflags(STR(0), (unsigned long)str2flags(chflags_flags, STR(1)));
> break;
> #endif
> case ACTION_TRUNCATE:
>
This excessive cast also causes a line longer than 80 characters, and doesn't
even match the prototype (lchflags() still takes an int arg for historical
reasons).
syscalls.master still misdeclares both chflags() and lchflags() and
some other syscalls as taking an int. For lchflags(), this is bug for
bug compatible with the prototype, so it works in practice even in the
big endian case, but for chflags() it would break in the same way as
for open() in the big endian case if longs were 64 bits. FreeBSD
supports some machines with 64 bit longs but all of these happen to
be little endian so chflags() works for all supported machines. Note
that chflags() couldn't be fixed by casting in the call -- the
prototype would still extend to 64 bits if longs are 64 bits.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090224172129.W10644>