xfs_admin: exit gracefully for non-XFS fileststems

PATCH In review Christoph Hellwig 3 participants 3 messages Jan 25, 2009 – Jan 29, 2009

Share & tools

Christoph Hellwig

author

2009-01-25 05:04 UTC #0 lore raw

PATCH 1/1 xfs_admin: exit gracefully for non-XFS fileststems

Exit gracefully if the magic number doesn't match xfs in xfs_admin and xfs_ncheck. This is implemented by a new -C option to xfs_db which makes it exit with a useful error message in case the filesystem doesn't match the XFS superblock magic number.

Reported-by Dick Middleton <[email protected]>

Signed-off-by Christoph Hellwig <[email protected]>

db/init.c +11 -2

Index: xfsprogs-dev/db/init.c
===================================================================
@@ -42,7 +42,7 @@ static void
 usage(void)
 {
 	fprintf(stderr, _(
-		"Usage: %s [-frxV] [-p prog] [-l logdev] [-c cmd]... device\n"),
+		"Usage: %s [-CfrxV] [-p prog] [-l logdev] [-c cmd]... device\n"),
 		progname);
 	exit(1);
 }

@@ -55,10 +55,14 @@ init(
 	xfs_sb_t	*sbp;
 	void		*bufp = NULL;
 	int		c;
+	int		check = 0;
 
 	progname = basename(argv[0]);
-	while ((c = getopt(argc, argv, "c:fip:rxVl:")) != EOF) {
+	while ((c = getopt(argc, argv, "c:Cfip:rxVl:")) != EOF) {
 		switch (c) {
+		case 'C':
+			check = 1;
+			break;
 		case 'c':
 			cmdline = xrealloc(cmdline, (ncmdline+1)*sizeof(char*));
 			cmdline[ncmdline++] = optarg;

@@ -118,6 +122,11 @@ init(
 
 	sbp = &xmount.m_sb;
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
+		if (check) {
+			fprintf(stderr, _("%s: not an XFS filesystem\n"),
+				progname);
+			exit(1);
+		}
 		fprintf(stderr, _("%s: unexpected XFS SB magic number 0x%08x\n"),
 			progname, sbp->sb_magicnum);
 	}

db/xfs_admin.sh +1 -1

Index: xfsprogs-dev/db/xfs_admin.sh
===================================================================
@@ -30,7 +30,7 @@ shift $OPTIND
 case $# in
 	1)	if [ -n "$DB_OPTS" ]
 		then
-			eval xfs_db -x -p xfs_admin $DB_OPTS $1
+			eval xfs_db -x -p xfs_admin -C $DB_OPTS $1
 			status=$?
 		fi
 		if [ -n "$REPAIR_OPTS" ]

db/xfs_ncheck.sh +1 -1

Index: xfsprogs-dev/db/xfs_ncheck.sh
===================================================================
@@ -25,7 +25,7 @@ done
 set -- extra $@
 shift $OPTIND
 case $# in
-	1)	xfs_db$DBOPTS -r -p xfs_ncheck -c "blockget -ns" -c "ncheck$OPTS" $1
+	1)	xfs_db$DBOPTS -r -p xfs_ncheck -C -c "blockget -ns" -c "ncheck$OPTS" $1
 		status=$?
 		;;
 	*)	echo $USAGE 1>&2

db/xfs_ncheck64.sh +1 -1

Index: xfsprogs-dev/db/xfs_ncheck64.sh
===================================================================
@@ -24,7 +24,7 @@ done
 set -- extra $@
 shift $OPTIND
 case $# in
-	1)	xfs_db64$DBOPTS -r -p xfs_ncheck64 -c "blockget -ns" -c "ncheck$OPTS" $1
+	1)	xfs_db64$DBOPTS -r -p xfs_ncheck64 -C -c "blockget -ns" -c "ncheck$OPTS" $1
 		status=$?
 		;;
 	*)	echo $USAGE 1>&2

man/man8/xfs_db.8 +6 -0

Index: xfsprogs-dev/man/man8/xfs_db.8
===================================================================
@@ -11,6 +11,8 @@ xfs_db \- debug an XFS filesystem
 ] [
 .B \-f
 ] [
+.B \-C
+] [
 .B \-l
 .I logdev
 ] [

@@ -32,6 +34,10 @@ that run
 .PP
 .SH OPTIONS
 .TP
+.B \-C
+Check the superblock magic number and exit gracefully if the filesystem is not
+an XFS filesystem.
+.TP
 .BI \-c " cmd"
 .B xfs_db
 commands may be run interactively (the default) or as arguments

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Josef 'Jeff' Sipek

2009-01-25 05:57 UTC #1 lore raw

Quoted 8 lines · Christoph Hellwig

Exit gracefully if the magic number doesn't match xfs in xfs_admin and xfs_ncheck. This is implemented by a new -C option to xfs_db which makes it exit with a useful error message in case the filesystem doesn't match the XFS superblock magic number.
Reported-by Dick Middleton <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>

...

Quoted 4 lines

Index: xfsprogs-dev/man/man8/xfs_db.8
===================================================================
man/man8/xfs_db.8 +0 -0

...

man/man8/xfs_db.8 that run · line 39

Quoted 7 lines

man/man8/xfs_db.8 +3 -0

@@ -32,6 +34,10 @@ that run
 .PP
 .SH OPTIONS
 .TP
+.B \-C
+Check the superblock magic number and exit gracefully if the filesystem is not
+an XFS filesystem.

Obviously, the 'C' is supposed to stand for "check". I think that's misleading. xfs_db _always_ checks the SB magic. This option just changes the the behavior on a mis-match. In a way, I think this flag belongs into the "verbose/quiet" category. It really is --you-really-want-to-work-with-xfs-volumes-only...I guess it's a don't-force flag.

I can't think of a good letter for it at the moment.

Josef 'Jeff' Sipek.

Signature · 7 lines

Hegh QaQ law'
quvHa'ghach QaQ puS

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2009-01-29 21:44 UTC #2 lore raw

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

Exit gracefully if the magic number doesn't match xfs in xfs_admin and xfs_ncheck. This is implemented by a new -C option to xfs_db which makes it exit with a useful error message in case the filesystem doesn't match the XFS superblock magic number.
Reported-by Dick Middleton <[email protected]>
Signed-off-by Christoph Hellwig <[email protected]>

I guess I'd rather see this the reverse, and require a flag to allow xfs_db to work on something that doesn't look like xfs - err on the side of safety (hm but I suppose db is usually in read-only mode...)

Addresses jeffpc's criticism too, to some degree; you can use -F to mean "force" or "foreign" :) before xfs_db will touch the fs.

So I think I personally like it better with the inverse, where it always exits on bad magic unless you explicitly tell it otherwise. Making the error message reference the option might be nice ("Use -F to ignore this error" or somesuch)

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs