cvs commit: src/sbin/fsck_ffs setup.c

[Wes Peters]

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
> <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

-- 

        Where am I, and what am I doing in this handbasket?

Wes Peters                                               wes at softweyr.com