Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 03, 2013 at 02:09:48PM -0500, Mark Tinguely wrote:
> On 06/03/13 00:28, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@redhat.com>
> >
> >When invalidating an attribute leaf block block, there might be
> >remote attributes that it points to. With the recent rework of the
> >remote attribute format, we have to make sure we calculate the
> >length of the attribute correctly. We aren't doing that in
> >xfs_attr3_leaf_inactive(), so fix it.
> >
> >Signed-off-by: Dave Chinner<dchinner@redhat.com>
> 
> I scratched my head reading:
> 
> in xfs_attr_leaf.h:
> /*
>  * Used to keep a list of "remote value" extents when unlinking an inode.
>  */
> typedef struct xfs_attr_inactive_list {
> 	xfs_dablk_t	valueblk;	/* block number of value bytes */
> 	int		valuelen;	/* number of bytes in value */
> 						     ^^^^^
> 						     |||||
> } xfs_attr_inactive_list_t;
> 
> Where "valuelen" is clearly being used as blocks.

Yeah, good point.

This is one of the reasons why I dislike comments explaining what
variables in structures mean. I didn't even look at the definition
of the structure, because it's meaning is obvious from the name of
the varaible of the code that uses it.  ;)

> A more obvious name is
> the former "valueblk". Blame commit d7929ff6 for the confusion.
> Should change
> the comment and/or variable one of these days ...

Actaully, a structure that is used once and local to a single
function shouldn't be declared in a header file - if should be local
to the function. I'll fix this in a separate patch for 3.11.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs