Date: Thu, 20 Nov 2003 07:17:08 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Sheldon Hearn <sheldonh@starjuice.net> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sbin/fsck_ffs setup.c Message-ID: <20031120064037.C8876@gamplex.bde.org> In-Reply-To: <20031119095138.GA752@starjuice.net> References: <200311160710.hAG7AtRR047311@repoman.freebsd.org> <20031119095138.GA752@starjuice.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20031120064037.C8876>