From owner-svn-src-all@FreeBSD.ORG Sun Nov 4 15:40:39 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 277AEE84; Sun, 4 Nov 2012 15:40:39 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au [211.29.132.183]) by mx1.freebsd.org (Postfix) with ESMTP id 9EC098FC0A; Sun, 4 Nov 2012 15:40:38 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qA4FeSK2017922 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 5 Nov 2012 02:40:29 +1100 Date: Mon, 5 Nov 2012 02:40:28 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek Subject: Re: svn commit: r242511 - head/sbin/fsck_msdosfs In-Reply-To: <20121104124843.GR1399@garage.freebsd.pl> Message-ID: <20121105000450.B4726@besplex.bde.org> References: <201211030918.qA39IcH6030403@svn.freebsd.org> <20121104124843.GR1399@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-Cloudmark-Score: 0 X-Optus-Cloudmark-Analysis: v=2.0 cv=eJBLFwV1 c=1 sm=1 a=8cLHmhZKkUMA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=cikS2bn0enQA:10 a=4PVSkj5MNWfP__btyhoA:9 a=CjuIK1q_8ugA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@freebsd.org, Jaakko Heinonen , svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 Nov 2012 15:40:39 -0000 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