From owner-cvs-src@FreeBSD.ORG Sat Nov 22 17:06:14 2003 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 98F7A16A4CE; Sat, 22 Nov 2003 17:06:14 -0800 (PST) Received: from smtp.omnis.com (smtp.omnis.com [216.239.128.26]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8EFE143FBF; Sat, 22 Nov 2003 17:06:13 -0800 (PST) (envelope-from wes@softweyr.com) Received: from softweyr.homeunix.net (66-91-236-204.san.rr.com [66.91.236.204]) by smtp-relay.omnis.com (Postfix) with ESMTP id 5E6E272E13; Sat, 22 Nov 2003 17:04:51 -0800 (PST) From: Wes Peters Organization: Softweyr To: Bruce Evans , Sheldon Hearn Date: Sat, 22 Nov 2003 17:06:04 -0800 User-Agent: KMail/1.5.4 References: <200311160710.hAG7AtRR047311@repoman.freebsd.org> <20031119095138.GA752@starjuice.net> <20031120064037.C8876@gamplex.bde.org> In-Reply-To: <20031120064037.C8876@gamplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200311221706.04729.wes@softweyr.com> cc: cvs-src@freebsd.org cc: src-committers@freebsd.org cc: re@freebsd.org cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sbin/fsck_ffs setup.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Nov 2003 01:06:14 -0000 RE, Bruce: This patch should be committed before 5.2 RELEASE. On Wednesday 19 November 2003 12:17 pm, Bruce Evans wrote: > 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 -- Where am I, and what am I doing in this handbasket? Wes Peters wes@softweyr.com