xfs
[Top] [All Lists]

Re: [PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 14 Apr 2010 16:19:12 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100412071131.GK2493@dastard>
References: <1270852266.7840.159.camel@doink> <20100412071131.GK2493@dastard>
Reply-to: aelder@xxxxxxx
On Mon, 2010-04-12 at 17:11 +1000, Dave Chinner wrote:
> On Fri, Apr 09, 2010 at 05:31:06PM -0500, Alex Elder wrote:
> > Minor things that didn't seem to warrant their own individual
> > patches:
> > - In xlog_bread_noalign(), reorder assertions so the buffer pointer
> >   is known to be non-null before attempting to dereference it.
> 
> That's OK, but if you are debugging then it'll be obvious what
> happend by it being a null ptr dereference rather than an assert
> failure. Hence if anything I'd just kill the ASSERT(bp)....

I personally prefer proactively asserting it rather than
just deducing it was a null pointer from the wreckage.  But
whatever, we disagree and I'm OK with dropping the assert.

> > - Add a more descriptive header comment for xlog_find_verify_cycle().
> 
> It just describes what the code does - I don't think it make the
> code any clearer and makes it more likely that if we ever change the
> code the comment will then be wrong...

I still think the existing comments are confusing.  What do you
think of this instead:

/*
 * Check that the range of blocks does not contain stop_on_cycle_no.
 * Fill in *new_blk with the block offset where such block is found,
 * or with -1 (an invalid block number) if there is no such block in
 * the range.  The scan needs to occur from front to back and the
 * pointer into the region must be updated since a later routine will
 * need to perform another test.  
 */

> > - Make a few additions to the comments in xlog_find_head().  Also
> >   rearrange some expressions in a few spots to produce the same
> >   result, but in a way that seems more clear what's being computed.
> 
> I'd say this is probably the only bits of the patch that add value.
> Can you split this one out by itself?

I will re-post a v3 on this and the others I've responded to
today, just so we're all clear on what's being accepted.  I
will break this patch into two pieces, allowing for this last
bit to be considered separate from the rest.

                                        -Alex

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