Date: Wed, 11 Feb 1998 07:30:03 -0800 (PST) From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs Subject: Re: bin/5711: bin/cat code cleanup Message-ID: <199802111530.HAA20849@hub.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/5711; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: freebsd-gnats-submit@FreeBSD.ORG, jason_smethers@bigfoot.com
Cc: Subject: Re: bin/5711: bin/cat code cleanup
Date: Thu, 12 Feb 1998 02:18:18 +1100
Review:
>diff -c -r /usr/src/bin/cat/cat.1 /usr/local/src/bin/cat/cat.1
>*** /usr/src/bin/cat/cat.1 Sat Feb 22 08:01:26 1997
>--- /usr/local/src/bin/cat/cat.1 Mon Feb 2 18:18:34 1998
>***************
>*** 13,19 ****
> .\" notice, this list of conditions and the following disclaimer in the
> .\" documentation and/or other materials provided with the distribution.
> .\" 3. All advertising materials mentioning features or use of this software
>! .\" must display the following acknowledgement:
> .\" This product includes software developed by the University of
> .\" California, Berkeley and its contributors.
> .\" 4. Neither the name of the University nor the names of its contributors
>--- 13,19 ----
> .\" notice, this list of conditions and the following disclaimer in the
> .\" documentation and/or other materials provided with the distribution.
> .\" 3. All advertising materials mentioning features or use of this software
>! .\" must display the following acknowledgment:
> .\" This product includes software developed by the University of
> .\" California, Berkeley and its contributors.
> .\" 4. Neither the name of the University nor the names of its contributors
Don't "fix" alternative spellings. "acknowledgment" is used in only
38 files in /usr/src; "acknowledgement" is used in 5148, mostly in
vendor (CSRG) copyrights. Do you really wish to "fix" them all? :-)
>--- 118,134 ----
>...
>! .Sh STANDARDS
>! The
>! .Nm cat
>! utility is compliant with the
>! .St -p1003.2-92
>! specification.
>...
"is compliant with" should be "conforms to".
>diff -c -r /usr/src/bin/cat/cat.c /usr/local/src/bin/cat/cat.c
>*** /usr/src/bin/cat/cat.c Fri Mar 28 09:24:04 1997
>--- /usr/local/src/bin/cat/cat.c Tue Feb 3 17:33:24 1998
>***************
>*** 63,68 ****
>--- 63,69 ----
> int rval;
> char *filename;
>
>+ int main __P((int, char *[]));
> void cook_args __P((char *argv[]));
> void cook_buf __P((FILE *));
> void raw_args __P((char *argv[]));
Lists of declarations should be ordered. Ordered lists shall remain
ordered.
>***************
>*** 96,107 ****
> tflag = vflag = 1; /* -t implies -v */
> break;
> case 'u':
>! setbuf(stdout, (char *)NULL);
> break;
> case 'v':
> vflag = 1;
> break;
> default:
> (void)fprintf(stderr,
> "usage: cat [-benstuv] [-] [file ...]\n");
> exit(1);
>--- 97,109 ----
> tflag = vflag = 1; /* -t implies -v */
> break;
> case 'u':
>! setbuf(stdout, NULL);
> break;
> case 'v':
> vflag = 1;
> break;
> default:
>+ case '?':
> (void)fprintf(stderr,
> "usage: cat [-benstuv] [-] [file ...]\n");
> exit(1);
Removing the cast just weakens K&R support.
>***************
>*** 248,258 ****
> err(1, "%s", filename);
> bsize = MAX(sbuf.st_blksize, 1024);
> if ((buf = malloc((u_int)bsize)) == NULL)
>! err(1, NULL);
> }
>! while ((nr = read(rfd, buf, bsize)) > 0)
> for (off = 0; nr; nr -= nw, off += nw)
>! if ((nw = write(wfd, buf + off, nr)) < 0)
> err(1, "stdout");
> if (nr < 0) {
> warn("%s", filename);
>--- 250,260 ----
> err(1, "%s", filename);
> bsize = MAX(sbuf.st_blksize, 1024);
> if ((buf = malloc((u_int)bsize)) == NULL)
>! err(1, "cannot allocate buffer");
> }
>! while ((nr = read(rfd, buf, (u_int)bsize)) > 0)
> for (off = 0; nr; nr -= nw, off += nw)
>! if ((nw = write(wfd, buf + off, (u_int)nr)) < 0)
> err(1, "stdout");
> if (nr < 0) {
> warn("%s", filename);
>
Don't add bogus casts. write()'s third arg has type size_t, not necessarily
u_int. Don't cast at all if possible; use variables with the correct type
instead. Here a complete cleanup requires at least:
(1) overflow handling for `bsize = MAX(sbuf.st_blksize, 1024);'. st_blksize
has type u_int32_t for stat() and int32_t for ostat(). bsize should
have type size_t and (to avoid bloat to handle portability problems)
value <= SSIZE_T_MAX and (to avoid using a non-power-of 2 size) a value
normally somewhat less than SSIZE_T_MAX. A simple implementation of
this:
size_t bsize;
...
bsize = MIN(sbuf.st_blksize, SSIZE_T_MAX);
if (bsize != sbuf.st_blksize) {
/*
* This shouldn't happen in practice, so don't bother
* finding the largest power of 2 <= SSIZE_T_MAX.
*/
assert(SSIZE_T_MAX >= 32767);
bsize = 16384;
}
/*
* Don't increase bsize to 1024 if sbuf.s_blksize < 1024, as in
* the original version. The system should know better than us
* whether a small size is best.
*/
2) Declare variables with the correct types. Since bsize < SSIZE_T_MAX,
we don't have to worry about read() or write() returning < 0 for a
non-error.
ssize_t nr, nw, off;
3) We still need to cast nr (to (size_t)) in the write() call to avoid
compiler warnings. The compiler can't be expected to know that nr >= 0,
since it can't be expected to know that write() returns a value less
than the amount requsted (modulo overflow problems which can't happen
here because bsize <= SSIZE_T_MAX), allthough it could know that the
initial nr is >= 0. Assigning nr to a variable with type size_t just
to avoid the cast would be worse than casting it.
4) Fix a non-cosmetic bug while we're here. bsize is static; it is
computed for the first call to raw_cat() and may be pessimal
or wrong (too small for a raw device) for subsequent calls.
See mv/mv.c:fastcopy() for fixes for this. Don't copy too much
from there. Type mismatches aren't handled there, and only regular
files are supported (short writes can be treated as errors since they
can't happen for regular files).
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199802111530.HAA20849>