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

Dave Chinner
2013-06-04T03:13:47Z

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