Date: Thu, 16 May 2002 17:40:01 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Giorgos Keramidas <keramida@FreeBSD.ORG> Cc: Mike Makonnen <makonnen@pacbell.net>, <freebsd-audit@FreeBSD.ORG> Subject: Re: RFC: Port of NetBSD cat(1)'s -f option. Message-ID: <20020516164332.B1704-100000@gamplex.bde.org> In-Reply-To: <20020515211758.GB68380@hades.hell.gr>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 16 May 2002, Giorgos Keramidas wrote: > In message: <1021491063.45509.645.camel@kokeb.ambesa.net> > Mike Makonnen <makonnen@pacbell.net> writes: > > > @@ -138,7 +141,15 @@ > > > filename = "stdin"; > > > fd = STDIN_FILENO; > > > } else { > > > + struct stat st; > > > + > > > filename = path; > > > + if (stat(path, &st) < 0 || > > > + S_ISREG(st.st_mode) == 0) { > > > + i++; /* Skip to next file. */ > > > + continue; > > > + } > > > + > > > > just a minor nit, > > > > You might want to show a warning, since stat(2) can fail for any number > > of reasons not related to the type of file. > > Sure, I've changed it to the following. Since this is a minor change, > I'll see that is gets done later tonight, if nobody objects 'til then. > > %%% > + if (stat(path, &st) < 0) { > + warn("%s", path); > + i++; /* Skip to next file. */ > + continue; > + } > + if (S_ISREG(st.st_mode) == 0) { > + i++; /* Skip to next file. */ > + continue; > + } > %%% I object. If stat() fails, then we have no idea about the file type, so we shouldn't classify it as regular and must set rval so that the error is reflected in cat's exit status. Letting the old code handle the error seems best. Using stat() instead of fstat() gives some races. NetBSD reduces the races by checking the mode both before and after open(). I didn't notice if your full patch does this (and deleted it :). Checking the mode again after open (after we have found fd > 0) would also fix the missing (?) check that stdin is a regular file. NetBSD doesn't check the mode of stdin but its man page says that -f causes non-plain files to be skipped. Checking the mode of stdin is also slightly wrong if stdin is refered to as /dev/stdin instead of "-". /dev/stdin is a (symlink to a) cdev, so it is never considered to be "plain". A more fundamental bug: The new variable fflag is never used. Style bugs: - nested declaration of st. - more verbose and bogus handling of the variable 'i'. 'i' is just the loop counter for a `for' loop that is obfuscated as a `while' loop. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020516164332.B1704-100000>