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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
