Re: [PATCH 03/13] xfs_db: add crc manipulation commands

[Brian Foster <bfoster@redhat.com>]

On Fri, Mar 20, 2015 at 09:30:13PM -0500, Eric Sandeen wrote:
> On 3/19/15 10:07 AM, Brian Foster wrote:
> > On Tue, Mar 17, 2015 at 03:33:05PM -0500, Eric Sandeen wrote:
> >> This adds a new "crc" command to xfs_db for CRC-enabled filesystems.
> ...
> 
> if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {
> > 
> > The 'if (argc)' thing strikes again. Does this address something I'm not
> > aware of?
> 
> just a dumb/unfortunate cut & paste from elsewhere, I'll fix.
> 
> ...
> 
> >> +		/* Off by one.. */
> >> +		crc = cpu_to_be32(crc + 1);
> >> +		setbitval(iocur_top->data, sfl->offset, bit_length, &crc);
> > 
> > Some comments on the above code would be nice to explain what it's
> > calculating.
> 
> Yeah, wish I'd written them "yonks ago" ;)  I'll reverse engineer my
> patch & add comments.
>  
> >> +		dbprintf(_("Verifying CRC:\n"));
> > 
> > Does the "verify" aspect of this command do anything, or are we just
> > printing the state that was determined from a previous cursor load? If
> > the latter, I wonder if it's worth retaining that option (e.g., just
> > print an inode to see the crc state)..?
> 
> If having a specific CRC handler makes sense (and, maybe it doesn't,
> with the other patch that allows corrupted write), the symmetry seemed
> nice.
> 

It's not really a big deal, it just wasn't quite clear. A comment would
help.

> Given the amount of code this adds, perhaps we should just drop it,
> TBH, and use the "write -c" functionality I added, instead.
> 

Hmm, well personally I'm not against having both. My initial reasoning
was to say that there's a difference between corrupting a structure
field and the crc. Thinking further, I suppose we can use the write -c
mechanism to write to the crc field itself. Given that and the type tree
gymnastics that this implementation has to do, I suppose I could see
leaning in favor of just write -c.

It's your call I guess. Maybe it's not worth it if you have to go back
through the type stuff and remember what it does to document it. :)

Brian

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

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