cvs commit: src/sbin/fsck_ffs setup.c

[Bruce Evans]

On Wed, 19 Nov 2003, Sheldon Hearn wrote:

> On (2003/11/15 23:10), Wes Peters wrote:
>
> >   FreeBSD src repository
> >
> >   Modified files:
> >     sbin/fsck_ffs        setup.c
> >   Log:
> >   Catch and report on filesystems that were interrupted during newfs,
> >   sporting the new 'BAD' magic number.  Exit with a unique error code
> >   (11) so callers who care about this can respond appropriately.
>
> Can you document this unique error code gracefully so that authors of
> such callers get clued in easily?

Note that the rule about error exits in style(9) was intended to say
not to use a huge number of meaningless undocumented sequentially
numbered error exits, as is done in some old programs like fsck_ffs.
The rule got broken to advise using the not-so-huge number of low-meaning
documented sequentially numbered error exits in <sysexits.h>.

This error exit is a bug IMO and doesn't exist in my version.  It prevents
readsb() returning to its caller so that the caller can either abort or
prompt for the next file system as appropriate.

This patch also fixes:
- old bug: silent premature termination of the search for a superblock
  after a read error.  Perhaps a read error should be immediately fatal.
  However, the non-searching case just returns for one.
- logic bug: as pointed out in my review, the search should be identical
  with the one in the kernel.  Bad magic number may be left lying
  around in harmless places by a previous failure followed by a newfs
  with different parameters.  Then the kernel will find a superblock
  but fsck would barf without this patch.  It would be useful to know
  about super blocks with the bad magic number so this version should
  be changed a bit to print a message before continuing, at least in
  the !preen case.
- old bug: the sanity check is not quite right here or in the kernel or
  in other ffs utilities.
  (a) There was no check that fs_bsize is not too large.  My change for
      this (to check MAXBSIZE) is not quite right.  The kernel must reject
      file systems whose block size is larger than vfs_bio can support,
      but utilities need not.  ffs systems with a block size larger than
      MAXBSIZE may be created on non-FreeBSD systems that have a larger
      value for this parameter.
  (b) The check that fs_bsize is larger than sizeof(struct fs) is not
      quite right.  On systems with MINBSIZE smaller than the FreeBSD
      value, newfs is happy to create file systems with the super block
      smaller than the block size, and such file systems almost work.
      I removed the check to allow testing such file systems and replace
      it by checks on fs_sbsize.

The failure cases in this patch have not all been tested at runtime.

%%%
Index: setup.c
===================================================================
RCS file: /home/ncvs/src/sbin/fsck_ffs/setup.c,v
retrieving revision 1.45
diff -u -2 -r1.45 setup.c
--- setup.c	16 Nov 2003 07:10:55 -0000	1.45
+++ setup.c	16 Nov 2003 11:29:27 -0000
@@ -300,5 +298,5 @@
 {
 	ufs2_daddr_t super;
-	int i;
+	int i, seenbad;

 	if (bflag) {
@@ -308,5 +306,5 @@
 		if (sblock.fs_magic == FS_BAD2_MAGIC) {
 			fprintf(stderr, BAD_MAGIC_MSG);
-			exit(11);
+			return (0);
 		}
 		if (sblock.fs_magic != FS_UFS1_MAGIC &&
@@ -317,13 +315,13 @@
 		}
 	} else {
+		seenbad = 0;
 		for (i = 0; sblock_try[i] != -1; i++) {
 			super = sblock_try[i] / dev_bsize;
 			if ((bread(fsreadfd, (char *)&sblock, super,
 			    (long)SBLOCKSIZE)))
-				return (0);
-			if (sblock.fs_magic == FS_BAD2_MAGIC) {
-				fprintf(stderr, BAD_MAGIC_MSG);
-				exit(11);
-			}
+				continue;
+			if (sblock.fs_magic == FS_BAD2_MAGIC)
+				/* XXX should we sanity check it too? */
+				seenbad = 1;
 			if ((sblock.fs_magic == FS_UFS1_MAGIC ||
 			     (sblock.fs_magic == FS_UFS2_MAGIC &&
@@ -331,9 +329,12 @@
 			    sblock.fs_ncg >= 1 &&
 			    sblock.fs_bsize >= MINBSIZE &&
-			    sblock.fs_bsize >= sizeof(struct fs))
+			    sblock.fs_bsize <= MAXBSIZE &&
+			    sblock.fs_sbsize >= (int)sizeof(sblock) &&
+			    sblock.fs_sbsize <= SBLOCKSIZE)
 				break;
 		}
 		if (sblock_try[i] == -1) {
-			fprintf(stderr, "Cannot find file system superblock\n");
+			fprintf(stderr, seenbad ? BAD_MAGIC_MSG :
+			    "Cannot find file system superblock\n");
 			return (0);
 		}
%%%

Bruce