[Ocfs2-tools-devel] [PATCH 01/10] Ocfs2-tools: Define new corrupt code to make it compatible with fsck.ocfs2.

[Joel Becker]

On Fri, Jun 05, 2009 at 03:45:34PM +0800, Tristan Ye wrote:
> In main.c, we redefine the corrupt_codes array to let it become FSCK_TYPE specific,
> which can be provided as comma-separated-values string to fswreck binary.
> 
> As our goal is to change the logic of corrupt operation as few as possible, their
> associated handling funcs will still use former corrupt_* func family.
> 
> Former CORRUPT_CODE enum will be abandoned as our new one will be more exact and
> specific.

	Nice start!  Some comments and improvements below.
>  /* remaining headers */
> diff --git a/fswreck/main.c b/fswreck/main.c
> index bc889d8..2690862 100644
> --- a/fswreck/main.c
> +++ b/fswreck/main.c
> @@ -30,72 +30,531 @@ char *progname = NULL;
>  static char *device = NULL;
>  static uint16_t slotnum = UINT16_MAX;
>  
> -struct corrupt_funcs {
> -	void (*func) (ocfs2_filesys *fs, int code, uint16_t slotnum);
> -	char *desc;
> +static int corrupt[NUM_FSCK_TYPE];
> +
> +struct prompt_code {
> +
> +	enum fsck_type type;
> +	char *str;
> +	void (*func)(ocfs2_filesys *fs, enum fsck_type code, uint16_t slotnum);
> +	char *desc;
>  };

	I like this structure.

> +static struct prompt_code prompt_codes[NUM_FSCK_TYPE] = {
> +
> +	{	.type = EB_BLKNO,
> +		.str = "EB_BLKNO",
> +		.func = &corrupt_file,
> +		.desc = "Extent block error: EB_BLKNO",
> +	},

	And this array.  But it's very verbose to type and read.  I
think you would be served by a macro that stringifies the type:

#define define_prompt_code(_type, _func, _desc)		\
	[_type]	= {					\
		.type = _type,				\
		.str = #_type,				\
		.func = _func,				\
		.desc = _desc,				\
	}

This allows you to say:

static struct prompt_code prompt_codes[NUM_FSCK_TYPE] = {
	define_prompt_code(EB_BLKNO, corrupt_file,
			   "Corrupt an extent block's eb_blkno field"),
	define_prompt_code(...),
	...,
};

	You don't need the '&' on function pointers.  The macro above
uses c99 initializers to make sure that the structure for _type is at
array index [_type], so they don't even need to be ordered properly
(though they still should be).  Finally, the description should be fully
readable.  Your code to print the description already prints ->str, so
you don't need it as part of ->desc.

> +	{	.type = EB_GEN,
> +		.str = "EB_GEN",
> +		.func = &corrupt_file,
> +		.desc = "Extent block error: EB_GEN",
> +	},

	For example, this is easier to understand as:

	define_prompt_code(EB_GEN, corrupt_file,
			   "Corrupt an extent blocks h_fs_generatoin field."),

> +	{	.type = EB_GEN_FIX,
> +		.str = "EB_GEN_FIX",
> +		.func = &corrupt_file,
> +		.desc = "Extent block error: EB_GEN_FIX",
> +	},

This one is a little more confusing.  It comes from the same error as
above, but it's all about triggering a fix of the extent block rather
than the specific corruption.  Perhaps:

	define_prompt_code(EB_GEN_FIX, corrupt_file,
			   "Corrupt an extent blocks h_fs_generatoin "
			   "field so that fsck.ocfs2 can fix it."),

> +	{
> +		.type = EXTENT_EB_INVALID,
> +		.str = "EXTENT_EB_INVALID",
> +		.func = &corrupt_file,
> +		.desc = "Extent block error: EXTENT_EB_INVALID",
> +	},

	Here I find an fswreck bug.  The fswreck EXTENT_EB_INVALID
corrupter actually does EB_GEN.  But the wrong fsck choice can make this
not exercise the EXTENT_EB_INVALID.  The corrupter for EXTENT_EB_INVALID
should instead have an extent record pointing to an unused block,
perhaps block 0.
	Overall, do a quick pass with these descriptions.  Don't work
too hard figuring them all out.  Once we've gotten the actual code
worked out, we can both work on making the descriptions better.

> @@ -110,8 +569,9 @@ static void usage (char *progname)
>  	g_print ("	-n <node slot number>\n");
>  	g_print ("	-c <corrupt code>\n");
>  	g_print ("	corrupt code description:\n");
> -	for (i = 0; i < MAX_CORRUPT; i++)
> -		g_print ("	%02d - %s\n", i, cf[i].desc);
> +	for (i = 0; i < NUM_FSCK_TYPE; i++)
> +		g_print("	%s - %s\n", prompt_codes[i].str,
> +			 prompt_codes[i].desc);

	See, you print str, desc.  So there's no need including the code
string in desc.
	Oh, can you change these to fprintf()?  fswreck doesn't use glib
anywhere else, and we can drop the dependency.

> +static int corrupt_code_match(const char *corrupt_codes)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_FSCK_TYPE; i++)
> +		if (strcmp(corrupt_codes, prompt_codes[i].str) == 0)
> +			return i;
> +
> +	return -1;
> +}

	Make 'corrupt_codes' the singular 'corrupt_code'.
parse_corrupt_codes() is passing a single code in to this function.
Just makes it easier to read.  You can even
assert(!strchr(corrupt_code, ',')) if you like.
	All this is just organizational.  The code looks very
functional.  Well done.

Joel

-- 

Life's Little Instruction Book #139

	"Never deprive someone of hope; it might be all they have."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127