Date: Sat, 22 Nov 2003 17:06:04 -0800 From: Wes Peters <wes@softweyr.com> To: Bruce Evans <bde@zeta.org.au>, Sheldon Hearn <sheldonh@starjuice.net> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sbin/fsck_ffs setup.c Message-ID: <200311221706.04729.wes@softweyr.com> In-Reply-To: <20031120064037.C8876@gamplex.bde.org> References: <200311160710.hAG7AtRR047311@repoman.freebsd.org> <20031119095138.GA752@starjuice.net> <20031120064037.C8876@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@softweyr.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200311221706.04729.wes>