From owner-cvs-all@FreeBSD.ORG Wed Nov 19 12:17:30 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 52DC916A4CE; Wed, 19 Nov 2003 12:17:30 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 044F243FE1; Wed, 19 Nov 2003 12:17:28 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id HAA15445; Thu, 20 Nov 2003 07:17:09 +1100 Date: Thu, 20 Nov 2003 07:17:08 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Sheldon Hearn In-Reply-To: <20031119095138.GA752@starjuice.net> Message-ID: <20031120064037.C8876@gamplex.bde.org> References: <200311160710.hAG7AtRR047311@repoman.freebsd.org> <20031119095138.GA752@starjuice.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@freebsd.org cc: src-committers@freebsd.org cc: Wes Peters cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sbin/fsck_ffs setup.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Nov 2003 20:17:30 -0000 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 . 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