Date: Mon, 5 Nov 2012 02:40:28 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@freebsd.org> Cc: svn-src-head@freebsd.org, Jaakko Heinonen <jh@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r242511 - head/sbin/fsck_msdosfs Message-ID: <20121105000450.B4726@besplex.bde.org> In-Reply-To: <20121104124843.GR1399@garage.freebsd.pl> References: <201211030918.qA39IcH6030403@svn.freebsd.org> <20121104124843.GR1399@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 4 Nov 2012, Pawel Jakub Dawidek wrote:
> On Sat, Nov 03, 2012 at 09:18:38AM +0000, Jaakko Heinonen wrote:
>> Log:
>> Print a newline after the error message.
>>
>> PR: bin/168447
>> Submitted by: Boris Kochergin
>>
>> Modified:
>> head/sbin/fsck_msdosfs/check.c
>>
>> Modified: head/sbin/fsck_msdosfs/check.c
>> ==============================================================================
>> --- head/sbin/fsck_msdosfs/check.c Sat Nov 3 04:56:08 2012 (r242510)
>> +++ head/sbin/fsck_msdosfs/check.c Sat Nov 3 09:18:37 2012 (r242511)
>> @@ -68,6 +68,7 @@ checkfilesys(const char *fname)
>>
>> if (dosfs < 0) {
>> perr("Can't open `%s'", fname);
>> + printf("\n");
>
> Why not just add \n to perr()? Doesn't perr() print at stderr, not
> stdout?
Because perr() has horrible API. Among other bugs, it depends on the
global variable `preen', and when this is set, it does the following:
- first, it always prints the message in format %s. The message must
be a string literal, since this API hasn't caught up with the
existence of vfprintf() yet. This bug is shared with perror(3).
- next, it prints strerror(errno) in format " (%s)". This bug is
shared with perror(3), except the format is gratuitously different
(for perror(3), the format is ": %s\n"). (Actually, it no longer
does this. So its reason for existence is broken. See below.)
- then if (preen), it prints a newline
- then if (preen), it prints a message with the device name and the
program name, in that unusual order, and exits
- the code has plenty of style bugs associated with this. For example,
the previous 2 checks of (preen) are done in separate if statements
though the condition for each is identical, and these closely-related
statements are sseparated by a newline. Almost the whole vmsg()
function is double-spaced.
So a newline in the message would be even wronger than for perror(3) --
it would first give a newline before the ' (%s)' for strerror(errno).
Then in the !preen case, there would still be no newline after the
messages. Dependency on the global makes it hard to see what happens.
No, perr() doesn't print on stderr.
These bugs were a little different until 2 weeks ago when perr() was
named from perror() and its API was changed significantly.
These bugs were much smaller in 4.4BSD. Note that perr() is an fsck
utility function that is used by fsck_msdosfs though it is not used by
fsck_ffs where it was derived from. In 4.4BSD, fsck_ffs was named
fsck and it had the the current utility functions pwarn(), pfatal()
and panic() with slightly different semantics, but not the current
utility function perr() which is more broken then the old ones. Some
of the differences are:
- in 4.4BSD, the 3 functions printed to the correct stream (stderr),
but other parts of fsck printed to stdout and stderr quite randomly.
This was "fixed" by printing more to stdout, but there are still
places that print to stderr.
- in 4.4BSD, none of the 3 functions printed strerror(errno)
- the 4.4BSD code is quite readable except for STDC ifdefs. Now there
is a vmsg() function to obfuscate all of this and add style bugs.
pfatal() is now vmsg() with fatal = 1 and pwarn() is now vmsg() with
fatal = 0. To add to the confusion, pfatal() is only actually
fatal if (preen). panic() is now the same as pfatal() followed by
exit(8) (it has to go through vmsg() and not pfatal() for technical
reasons). These end result of these 3 functions hasn't changed
significantly. Another insignficant difference is that the program
name is now not hard-coded as "fsck".
- the above is out of date for perr(). When its name was changed
from perror(), its action was changed to not print strerror(errno).
Now it is identical to pfatal() except for its name. It has even
caught up with the existence of vfprintf(), so it now accepts an
arg list like the other 3 functions. A newline in the message
would no longer break the formatting of strerror(errno). It would
just give a doubled newline in the actually-fatal (preen) case.
Clearly the existence of perr() is a bug, but it has to remain in
fsck/fs_utility.c for compatibility until its callers are changed.
Most of its callers are wrong anyway:
- fsck_ffs knows better than to call it. But fsck_ffs is broken in
another way, by mixing streams using lots of errx() and err() calls.
There were lots of errx() calls in 4.4BSD too. Last time I checked,
this stream mixing was not too bad -- mostly the errx() calls are
for up-front error checking where there has probably been no previous
output to stdout.
- fsck (the driver utility) used to call perror() just once outside of
the utilities functions. It now calls perr() there. This call is
because it is for a malloc() failure which will cause a null pointer
dereference soon if perr() returns. And of course the message is
missing a newline.
fsck used to call perror() in the utilities functions, but these
calls were changed to simply call perr(). This made them more broken
than before. They are implemented next to the bad API so they should
know what it is. They used to know, so they were careful about the
newline, but they no longer are:
% Index: fsutil.c
% ===================================================================
% RCS file: /home/ncvs/src/sbin/fsck/fsutil.c,v
% retrieving revision 1.8
% retrieving revision 1.9
% diff -u -2 -r1.8 -r1.9
% --- fsutil.c 9 Apr 2004 19:58:28 -0000 1.8
% +++ fsutil.c 21 Oct 2012 12:01:11 -0000 1.9
% @@ -133,16 +137,13 @@
%
% if (stat("/", &stslash) < 0) {
% - perror("/");
% - printf("Can't stat root\n");
% + perr("Can't stat `/'");
% return (origname);
% }
perror(3) is hard to use, so there the message may have to be partly
in the perror() arg and partly in another line after perror() returns.
This works even better if perror() is not perror(3) and doesn't print
a newline. This care has been lost, and the above now doesn't print
the newline if perr() returns.
% if (stat(origname, &stchar) < 0) {
% - perror(origname);
% - printf("Can't stat %s\n", origname);
% + perr("Can't stat %s\n", origname);
% return (origname);
% }
% if (!S_ISCHR(stchar.st_mode)) {
% - perror(origname);
% - printf("%s is not a char device\n", origname);
% + perr("%s is not a char device\n", origname);
% }
% return (origname);
%
Now the newline is in the message, so it gives the opposite problem --
an extra newline if perr() doesn't return.
- fsck_msdosfs. This uses perr() a lot. But it doesn't actually understand
fsck's perror(), so it almost never printed a newline after calling
it. After the recent commit, it now prints a newline after calling
it in 1 more place. It still thinks that fsck's perror() is bug for
bug compatible with perror(3), so it is careful to not put newlines
in its messages. The result is that the calls work iff they are
fatal (if (preen)), except they are now missing strerror(errno) which
is useful in some cases.
fsck_msdosfs also uses pwarn() and pfatal() a lot, but never panic().
It knows to supply the newline in or after the message for pwarn()
and pfatal(). The distinction is apparently that perror() is used
when strerror(errno) is wanted and pwarn() when an exit is not wanted,
but there are a few cases where perror() is used when strerror(errno)
is not wanted. It knows that pfatal() is often not actually has fatal,
and has strange-looking code like:
pfatal("whatever\n");
return FSFATAL;
It mostly puts the newline in the message like this. fsck_ffs often
dances in a more complicated way with this newline formatting. It
does things like:
@ pfatal("lost+found IS NOT A DIRECTORY");
@ if (reply("REALLOCATE") == 0)
@ return (0);
It's good to have the question on the same line as the pfatal() message.
It can be hard to keep track of the number of newlines that your subroutines
printed in code like this.
@ oldlfdir = lfdir;
@ if ((lfdir = allocdir(ROOTINO, (ino_t)0, lfmode)) == 0) {
@ pfatal("SORRY. CANNOT CREATE lost+found DIRECTORY\n\n");
@ return (0);
@ }
I don't like the doubled newlines. They only occur in dir.c (with 3
instances, 2 quoted here. I didn't choose these as especially bad
examples) At least they aren't tripled due to pfatal() adding another
one, since this case is interactive so it must be !preen.
@ ...
@ if (makeentry(lfdir, orphan, (name ? name : tempname)) == 0) {
@ pfatal("SORRY. NO SPACE IN lost+found DIRECTORY");
@ printf("\n\n");
@ return (0);
@ }
No message here, and the doubled newline separate from the message, unlike
in the above.
Another difference between fsck_msdosfs and fsck_ffs in use of these
functions is that the latter more often uses reply() after pfatal()
than the former uses ask() after pfatal(). It only makes sense to ask
after pwarn(). In the !preen case where asking is done, pfatal() is
not actually fatal and is identical to pwarn(). Except, oops, I was
looking in the wrong place for fsck_ffs's functions for this. The
general ones are so unusable that fsck_ffs never uses any of them --
it has its own set in fsck_ffs/fsutils.c. Except for pfatal(), these
are essentially(?) just the old ones from 4.4BSD, with the STDC ifdefs
removed and a style bug (missing blank line after declarations) added
where they were. pfatal() now does much more than printing and possibly
not actually being fatal -- it now handles starting and recovering
from background fsck.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121105000450.B4726>