xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 03/13] xfs_db: add crc manipulation commands
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Sat, 21 Mar 2015 10:18:54 -0400
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <550CD7B5.8030507@xxxxxxxxxxx>
References: <1426624395-8258-1-git-send-email-sandeen@xxxxxxxxxx> <1426624395-8258-4-git-send-email-sandeen@xxxxxxxxxx> <20150319150755.GC11669@xxxxxxxxxxxxxx> <550CD7B5.8030507@xxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>