xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH 03/13] xfs_db: add crc manipulation commands
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 20 Mar 2015 21:30:13 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150319150755.GC11669@xxxxxxxxxxxxxx>
References: <1426624395-8258-1-git-send-email-sandeen@xxxxxxxxxx> <1426624395-8258-4-git-send-email-sandeen@xxxxxxxxxx> <20150319150755.GC11669@xxxxxxxxxxxxxx>
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.

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

-Eric

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