Re: [PATCH 09/21] implement generic xfs_btree_increment

[Christoph Hellwig <hch@lst.de>]

On Wed, Jul 30, 2008 at 12:06:45PM +1000, Dave Chinner wrote:
> > +int xfs_btree_increment(struct xfs_btree_cur *, int, int *);
> 
> I think for these interfaces it woul dbe better to include
> variable names in the prototypes. i.e.:
> 
> int xfs_btree_increment(struct xfs_btree_cur *cur, int level, int *stat);

Well, all this and much more is documented at the implementation site,
so I'd avoid this churn.  If there are strong feelings for it I
can change the prototypes.

> Can kill the else there:
> 
> 	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
> 		return be64_to_cpu(ptr->l) == NULLFSBLOCK;
> 	return be32_to_cpu(ptr->s) == NULLAGBLOCK;

Normally I'd agree with you, but for the short/long pointer case the
if else make the symmetry of the code clear and thus is IMHO better
readable.

> > +			ptr->s = block->bb_u.s.bb_rightsib;
> > +		else
> > +			ptr->s = block->bb_u.s.bb_leftsib;
> > +	}
> > +}
> 
> Should we use trinary notation for this? i.e:

I find the if else much easier to read.

> Hmmm - if xfs_btree_check_block() returns an error, we won't release
> the buffer in the error handling path. While this may not be a
> problem right now (as the only error is EFSCORRUPTED) we want to be
> able to recover from errors here in the future so we should really
> exit cleanly form this function....

Fixed.

> Would it be better here to explicitly test this rather than assert?
> ie.:
> 
> 	if (lev == cur->bc_nlevels) {
> 		if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
> 			goto out0;
> 		ASSERT(0);
> 		error = EFSCORRUPTED;
> 		goto error0;
> 	}
> 
> So that we get an explicit error reported in the production systems
> rather than carrying on and dying a horrible death somewhere else....

Yes, this is much better.  But we're getting on a slipperly slope here
to introduce too many improvements.  For the initial patches I'd like
to stay as close as possible to the old btree implementations.

Anyway, this change is trivial enough and I've added it.