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>