diff mbox

[1/2] have e2fsck/problem.c verify problem.h error codes

Message ID 20090427225924.GG3209@webber.adilger.int
State Accepted, archived
Headers show

Commit Message

Andreas Dilger April 27, 2009, 10:59 p.m. UTC 
We've hit a number of cases where the error codes in problem.h have
been assigned duplicate values compared to problems in our own e2fsck
patches, and this can lead to confusing and difficult to find bugs
in e2fsck (e.g. wrong problem messages, incorrect repair action, etc).

Attached is a test case for the problem.c file to ensure that the
problem table is sorted and does not contain any duplicate values.
Having the problem table sorted allows the correctness checking to be
very simple, and if it ever became important for performance we could
use binary searching of the problem table for the specific problem code.

Signed-off-by: Andreas Dilger <adilger@sun.com>


Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Theodore Ts'o April 28, 2009, 2:08 a.m. UTC | #1 
On Mon, Apr 27, 2009 at 04:59:24PM -0600, Andreas Dilger wrote:
> We've hit a number of cases where the error codes in problem.h have
> been assigned duplicate values compared to problems in our own e2fsck
> patches, and this can lead to confusing and difficult to find bugs
> in e2fsck (e.g. wrong problem messages, incorrect repair action, etc).
> 
> Attached is a test case for the problem.c file to ensure that the
> problem table is sorted and does not contain any duplicate values.
> Having the problem table sorted allows the correctness checking to be
> very simple, and if it ever became important for performance we could
> use binary searching of the problem table for the specific problem code.

Hmm, I wonder if we should be doing this a different way.  Maybe what
we should do is to have a single file, call it problem_codes.in, that
has a format somewhat like this:

DEFINE_PROBLEM(PR_1_ROOT_NO_DIR, 0x010001, "@r is not a @d.  ", 
	       PROMPT_CLEAR, 0)

... which then generates problem_code.h and problem_code.c.  That way
there is a single place where these things are defined, and it
wouldn't be that hard to create a perl script which looks for multiply
assigned problem assignments.

In the short term, we can also code up the test script much more
simply:

awk '/^#define/ {print $3}' < e2fsck/problem.h   | sort | uniq -d

    		       	      			   - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger April 28, 2009, 7:03 p.m. UTC | #2 
On Apr 27, 2009  22:08 -0400, Theodore Ts'o wrote:
> On Mon, Apr 27, 2009 at 04:59:24PM -0600, Andreas Dilger wrote:
> > We've hit a number of cases where the error codes in problem.h have
> > been assigned duplicate values compared to problems in our own e2fsck
> > patches, and this can lead to confusing and difficult to find bugs
> > in e2fsck (e.g. wrong problem messages, incorrect repair action, etc).
> > 
> > Attached is a test case for the problem.c file to ensure that the
> > problem table is sorted and does not contain any duplicate values.
> > Having the problem table sorted allows the correctness checking to be
> > very simple, and if it ever became important for performance we could
> > use binary searching of the problem table for the specific problem code.
> 
> Hmm, I wonder if we should be doing this a different way.  Maybe what
> we should do is to have a single file, call it problem_codes.in, that
> has a format somewhat like this:
> 
> DEFINE_PROBLEM(PR_1_ROOT_NO_DIR, 0x010001, "@r is not a @d.  ", 
> 	       PROMPT_CLEAR, 0)

I was wondering the same, but that would involve a huge amount of
code churn and it probably isn't something you'd accept from someone
else in the end...  It would still likely be desirable to have some
indicator of which phase the problem is related to.

> ... which then generates problem_code.h and problem_code.c.  That way
> there is a single place where these things are defined, and it
> wouldn't be that hard to create a perl script which looks for multiply
> assigned problem assignments.
> 
> In the short term, we can also code up the test script much more
> simply:
> 
> awk '/^#define/ {print $3}' < e2fsck/problem.h   | sort | uniq -d

Sure, but my patch exists now, and doesn't add any overhead to the
runtime.  The fact that the test keeps the table sorted also allows
the ability to do a binary search on the table, should we want to.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o May 3, 2009, 1:11 a.m. UTC | #3 
Thanks, I've applied this patch to e2fsprogs.

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: e2fsprogs-1.41.5/e2fsck/problem.c
===================================================================
--- e2fsprogs-1.41.5.orig/e2fsck/problem.c
+++ e2fsprogs-1.41.5/e2fsck/problem.c
@@ -240,15 +240,10 @@  static struct e2fsck_problem problem_tab
 	  N_("Clear @j"),
 	  PROMPT_NULL, PR_PREEN_NOMSG },
 
-	/* Ask if we should run the journal anyway */
-	{ PR_0_JOURNAL_RUN,
-	  N_("Run @j anyway"),
-	  PROMPT_NULL, 0 },
-
-	/* Run the journal by default */
-	{ PR_0_JOURNAL_RUN_DEFAULT,
-	  N_("Recovery flag not set in backup @S, so running @j anyway.\n"),
-	  PROMPT_NONE, 0 },
+	/* Filesystem revision is 0, but feature flags are set */
+	{ PR_0_FS_REV_LEVEL,
+	  N_("@f has feature flag(s) set, but is a revision 0 @f.  "),
+	  PROMPT_FIX, PR_PREEN_OK | PR_NO_OK },
 
 	/* Clearing orphan inode */
 	{ PR_0_ORPHAN_CLEAR_INODE,
@@ -275,11 +270,6 @@  static struct e2fsck_problem problem_tab
 	  N_("@I @i %i in @o @i list.\n"),
 	  PROMPT_NONE, 0 },
 
-	/* Filesystem revision is 0, but feature flags are set */
-	{ PR_0_FS_REV_LEVEL,
-	  N_("@f has feature flag(s) set, but is a revision 0 @f.  "),
-	  PROMPT_FIX, PR_PREEN_OK | PR_NO_OK },
-
 	/* Journal superblock has an unknown read-only feature flag set */
 	{ PR_0_JOURNAL_UNSUPP_ROCOMPAT,
 	  N_("@j @S has an unknown read-only feature flag set.\n"),
@@ -311,6 +301,16 @@  static struct e2fsck_problem problem_tab
 	     "Clearing fields beyond the V1 @j @S...\n\n"),
 	  PROMPT_NONE, 0 },
 
+	/* Ask if we should run the journal anyway */
+	{ PR_0_JOURNAL_RUN,
+	  N_("Run @j anyway"),
+	  PROMPT_NULL, 0 },
+
+	/* Run the journal by default */
+	{ PR_0_JOURNAL_RUN_DEFAULT,
+	  N_("Recovery flag not set in backup @S, so running @j anyway.\n"),
+	  PROMPT_NONE, 0 },
+
 	/* Backup journal inode blocks */
 	{ PR_0_BACKUP_JNL,
 	  N_("Backing up @j @i @b information.\n\n"),
@@ -793,11 +793,6 @@  static struct e2fsck_problem problem_tab
 	  N_("@a in @i %i has a namelen (%N) which is @n\n"),
 	  PROMPT_CLEAR, PR_PREEN_OK },
 
-	/* invalid ea entry->e_value_size */
-	{ PR_1_ATTR_VALUE_SIZE,
-	  N_("@a in @i %i has a value size (%N) which is @n\n"),
-	  PROMPT_CLEAR, PR_PREEN_OK },
-
 	/* invalid ea entry->e_value_offs */
 	{ PR_1_ATTR_VALUE_OFFSET,
 	  N_("@a in @i %i has a value offset (%N) which is @n\n"),
@@ -808,6 +803,11 @@  static struct e2fsck_problem problem_tab
 	  N_("@a in @i %i has a value @b (%N) which is @n (must be 0)\n"),
 	  PROMPT_CLEAR, PR_PREEN_OK },
 
+	/* invalid ea entry->e_value_size */
+	{ PR_1_ATTR_VALUE_SIZE,
+	  N_("@a in @i %i has a value size (%N) which is @n\n"),
+	  PROMPT_CLEAR, PR_PREEN_OK },
+
 	/* invalid ea entry->e_hash */
 	{ PR_1_ATTR_HASH,
 	  N_("@a in @i %i has a hash (%N) which is @n\n"),
@@ -1597,11 +1597,6 @@  static struct e2fsck_problem problem_tab
 	  " +(%i--%j)",
 	  PROMPT_NONE, PR_LATCH_IBITMAP | PR_PREEN_OK | PR_PREEN_NOMSG },
 
-	/* Recreate journal if E2F_FLAG_JOURNAL_INODE flag is set */
-	{ PR_6_RECREATE_JOURNAL,
-	  N_("Recreate @j"),
-	  PROMPT_NULL, PR_PREEN_OK | PR_NO_OK },
-
 	/* Group N block(s) in use but group is marked BLOCK_UNINIT */
 	{ PR_5_BLOCK_UNINIT,
 	  N_("@g %g @b(s) in use but @g is marked BLOCK_UNINIT\n"),
@@ -1612,6 +1607,13 @@  static struct e2fsck_problem problem_tab
 	  N_("@g %g @i(s) in use but @g is marked INODE_UNINIT\n"),
 	  PROMPT_FIX, PR_PREEN_OK },
 
+	/* Post-Pass 5 errors */
+
+	/* Recreate journal if E2F_FLAG_JOURNAL_INODE flag is set */
+	{ PR_6_RECREATE_JOURNAL,
+	  N_("Recreate @j"),
+	  PROMPT_NULL, PR_PREEN_OK | PR_NO_OK },
+
 	{ 0 }
 };
 
@@ -1834,3 +1835,89 @@  int fix_problem(e2fsck_t ctx, problem_t 
 
 	return answer;
 }
+
+#ifdef UNITTEST
+
+#include <stdlib.h>
+#include <stdio.h>
+
+errcode_t
+profile_get_boolean(profile_t profile, const char *name, const char *subname,
+		    const char *subsubname, int def_val, int *ret_boolean)
+{
+	return 0;
+}
+
+void print_e2fsck_message(e2fsck_t ctx, const char *msg,
+			  struct problem_context *pctx, int first,
+			  int recurse)
+{
+	return;
+}
+
+void fatal_error(e2fsck_t ctx, const char *msg)
+{
+	return;
+}
+
+void preenhalt(e2fsck_t ctx)
+{
+	return;
+}
+
+errcode_t
+profile_get_string(profile_t profile, const char *name, const char *subname,
+		   const char *subsubname, const char *def_val,
+		   char **ret_string)
+{
+	return 0;
+}
+
+int ask (e2fsck_t ctx, const char * string, int def)
+{
+	return 0;
+}
+
+int verify_problem_table(e2fsck_t ctx)
+{
+	struct e2fsck_problem *curr, *prev = NULL;
+	int rc = 0;
+
+	for (prev = NULL, curr = problem_table; curr->e2p_code; prev = curr++) {
+		if (prev == NULL)
+			continue;
+
+		if (curr->e2p_code > prev->e2p_code)
+			continue;
+
+		if (curr->e2p_code == prev->e2p_code)
+			fprintf(stderr, "*** Duplicate in problem table:\n");
+		else
+			fprintf(stderr, "*** Unordered problem table:\n");
+
+		fprintf(stderr, "curr code = 0x%08x: %s\n",
+			curr->e2p_code, curr->e2p_description);
+		fprintf(stderr, "*** prev code = 0x%08x: %s\n",
+			prev->e2p_code, prev->e2p_description);
+
+		fprintf(stderr, "*** This is a %sprogramming error in e2fsck\n",
+			(curr->e2p_code == prev->e2p_code) ? "fatal " : "");
+
+		rc = 1;
+	}
+
+	return rc;
+}
+
+int main(int argc, char *argv[])
+{
+	e2fsck_t ctx;
+	int rc;
+
+	rc = verify_problem_table(ctx);
+	if (rc == 0)
+		printf("e2fsck problem table verified\n");
+
+	return rc;
+}
+#endif /* UNITTEST */
Index: e2fsprogs-1.41.5/e2fsck/problem.h
===================================================================
--- e2fsprogs-1.41.5.orig/e2fsck/problem.h
+++ e2fsprogs-1.41.5/e2fsck/problem.h
@@ -529,10 +529,10 @@  struct problem_context {
 #define PR_1B_ALLOCATE_IBITMAP_ERROR 0x011005
 
 /* Error while iterating over blocks */
-#define PR_1B_BLOCK_ITERATE	0x0110006
+#define PR_1B_BLOCK_ITERATE	0x011006
 
 /* Error adjusting EA refcount */
-#define PR_1B_ADJ_EA_REFCOUNT	0x0110007
+#define PR_1B_ADJ_EA_REFCOUNT	0x011007
 
 
 /* Pass 1C: Scan directories for inodes with dup blocks. */
Index: e2fsprogs-1.41.5/e2fsck/Makefile.in
===================================================================
--- e2fsprogs-1.41.5.orig/e2fsck/Makefile.in
+++ e2fsprogs-1.41.5/e2fsck/Makefile.in
@@ -134,6 +134,10 @@  crc32table.h: gen_crc32table
 	@echo "	GEN32TABLE $@"
 	@./gen_crc32table > crc32table.h
 
+tst_problem: $(srcdir)/problem.c $(srcdir)/problem.h $(LIBEXT2FS)
+	@$(CC) $(BUILD_LDFLAGS) $(ALL_CFLAGS) -o tst_problem \
+		$(srcdir)/problem.c -DUNITTEST $(LIBEXT2FS) $(LIBCOM_ERR)
+
 tst_crc32: $(srcdir)/crc32.c $(LIBEXT2FS)
 	@$(CC) $(BUILD_LDFLAGS) $(ALL_CFLAGS) -o tst_crc32 $(srcdir)/crc32.c \
 		-DUNITTEST $(LIBEXT2FS) $(LIBCOM_ERR)
@@ -148,10 +152,11 @@  tst_region: region.c
 	@$(CC) -o tst_region $(srcdir)/region.c \
 		$(ALL_CFLAGS) -DTEST_PROGRAM $(LIBCOM_ERR)
 
-check:: tst_refcount tst_region tst_crc32
+check:: tst_refcount tst_region tst_crc32 tst_problem
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_refcount
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_region
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_crc32
+	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_problem
 
 extend: extend.o
 	@echo "	LD $@"
@@ -261,7 +266,7 @@  uninstall:
 clean:
 	$(RM) -f $(PROGS) \#* *\# *.s *.o *.a *~ core e2fsck.static \
 		e2fsck.shared e2fsck.profiled flushb e2fsck.8 \
-		tst_crc32 tst_region tst_refcount gen_crc32table \
+		tst_problem tst_crc32 tst_region tst_refcount gen_crc32table \
 		crc32table.h e2fsck.conf.5 prof_err.c prof_err.h \
 		test_profile
 	$(RM) -rf profiled