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
|