Re: [PATCH] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers

[Eric Sandeen <sandeen@sandeen.net>]

On 3/13/15 8:43 AM, Brian Foster wrote:
> On Thu, Mar 05, 2015 at 03:26:32PM -0600, Eric Sandeen wrote:
>> Being able to write corrupt data is handy if we wish to test
>> repair against specific types of corruptions.
>>
>> Add a "-c" option to the write command which allows this.
>>
>> Note that this also skips CRC updates; it's not currently possible
>> to write invalid data with a valid CRC; CRC recalculation is
>> intertwined with validation.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/db/crc.c b/db/crc.c
>> index ad46c3f..861086a 100644
>> --- a/db/crc.c
>> +++ b/db/crc.c
> 
> This depends on the previous crc command which isn't merged yet. It
> might be worth it to include that again here so it hopefully gets picked
> up.
> 
>> @@ -58,13 +58,6 @@ crc_help(void)
>>  
>>  }
>>  
>> -void
>> -xfs_dummy_verify(
>> -	struct xfs_buf *bp)
>> -{
>> -	return;
>> -}
>> -
>>  static int
>>  crc_f(
>>  	int		argc,
>> diff --git a/db/crc.h b/db/crc.h
>> index 80ecec3..9f44615 100644
>> --- a/db/crc.h
>> +++ b/db/crc.h
>> @@ -20,4 +20,3 @@ struct field;
>>  
>>  extern void	crc_init(void);
>>  extern void	crc_struct(const field_t *fields, int argc, char **argv);
>> -extern void	xfs_dummy_verify(struct xfs_buf *bp);
>> diff --git a/db/io.c b/db/io.c
>> index eb3daa1..2d18d56 100644
>> --- a/db/io.c
>> +++ b/db/io.c
>> @@ -458,6 +458,13 @@ write_cur_bbs(void)
>>  }
>>  
>>  void
>> +xfs_dummy_verify(
>> +	struct xfs_buf *bp)
>> +{
>> +	return;
>> +}
>> +
>> +void
>>  write_cur(void)
>>  {
>>  	int skip_crc = (iocur_top->bp->b_ops->verify_write == xfs_dummy_verify);
>> diff --git a/db/io.h b/db/io.h
>> index 71082e6..31d96b4 100644
>> --- a/db/io.h
>> +++ b/db/io.h
>> @@ -63,6 +63,7 @@ extern void	set_cur(const struct typ *t, __int64_t d, int c, int ring_add,
>>  			bbmap_t *bbmap);
>>  extern void     ring_add(void);
>>  extern void	set_iocur_type(const struct typ *t);
>> +extern void	xfs_dummy_verify(struct xfs_buf *bp);
>>  
>>  /*
>>   * returns -1 for unchecked, 0 for bad and 1 for good
>> diff --git a/db/write.c b/db/write.c
>> index a0f14f4..a01c25a 100644
>> --- a/db/write.c
>> +++ b/db/write.c
>> @@ -38,7 +38,7 @@ static int	write_f(int argc, char **argv);
>>  static void     write_help(void);
>>  
>>  static const cmdinfo_t	write_cmd =
>> -	{ "write", NULL, write_f, 0, -1, 0, N_("[field or value]..."),
>> +	{ "write", NULL, write_f, 0, -1, 0, N_("[-c] [field or value]..."),
>>  	  N_("write value to disk"), write_help };
>>  
>>  void
>> @@ -79,6 +79,7 @@ write_help(void)
>>  "  String mode: 'write \"This_is_a_filename\" - write null terminated string.\n"
>>  "\n"
>>  " In data mode type 'write' by itself for a list of specific commands.\n\n"
>> +" Specifying the -c option will allow writes of invalid (corrupt) data.\n\n"
>>  ));
>>  
>>  }
>> @@ -90,6 +91,9 @@ write_f(
>>  {
>>  	pfunc_t	pf;
>>  	extern char *progname;
>> +	int c;
>> +	int corrupt = 0;		/* Allow write of corrupt data; skip verification */
>> +	const struct xfs_buf_ops *stashed_ops = NULL; 
> 
> Trailing space here.
> 
>>  
>>  	if (x.isreadonly & LIBXFS_ISREADONLY) {
>>  		dbprintf(_("%s started in read only mode, writing disabled\n"),
>> @@ -109,12 +113,36 @@ write_f(
>>  		return 0;
>>  	}
>>  
>> -	/* move past the "write" command */
>> -	argc--;
>> -	argv++;
>> +	if (argc) while ((c = getopt(argc, argv, "c")) != EOF) {
> 
> Isn't argc always going to be non-zero at this point (e.g., argv[0] ==
> "write")?

Hm, yeah, not sure if that was a thinko or a typo, thanks.

(hm, db's bmap_f does the same thing, I might have just blindly copied that)

>> +	if (corrupt) {
>> +		struct xfs_buf_ops nowrite_ops;
>> +
> 
> This may not be an explicit failure, but I'd rather not define this on
> the stack in the local scope of this branch and then implicitly use it
> outside of that scope. Could we move the definition and possibility the
> initialization to the top of the function?

*nod* I'll fix that too.

I'll send the crc patch & this in another 2-patch series (though I am wondering
if the crc patch is needed, if we're able to write bad data; it kind of achieves
the same goal - thoughts?)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs