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

[tristan.ye]

On Fri, 2009-06-05 at 12:57 -0700, Joel Becker wrote:
> 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.

Thank you joel, Your suggestion sounds great as I also was tired of
typing such a long array definition...

> 
> > +	{	.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.

Yes, I'll later make the descs much more readable and understandable.

> 
> > @@ -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.

I meant to make the print function unchanged, Ok, that's fine to use
frprintf here.

> 
> > +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.

Nod:)

> 	All this is just organizational.  The code looks very
> functional.  Well done.
> 
> Joel
>