Re: [PATCH] (and bad attr2 bug) - pack xfs_sb_t for 64-bit arches

[David Chinner <dgc@sgi.com>]

On Fri, Nov 17, 2006 at 02:11:12PM +1000, Timothy Shimmin wrote:
> >so it translates is as though it was a 64 bit field,
> >not a 32 bit field.....
> >
> 
> So why not change xfs_sb_info to give the real offset of where
> the next field should go (if there was one), instead of giving the sizeof 
> the
> structure which is not where say a 32 bit field would go and
> is wrong IMHO.
> 
> i.e.
> 
> ===========================================================================
> Index: fs/xfs/xfs_mount.c
> ===========================================================================
> 
> --- a/fs/xfs/xfs_mount.c	2006-11-17 15:02:21.000000000 +1100
> +++ b/fs/xfs/xfs_mount.c	2006-11-17 14:48:43.261937705 +1100
> @@ -121,7 +121,7 @@ static const struct {
>     { offsetof(xfs_sb_t, sb_logsectsize),0 },
>     { offsetof(xfs_sb_t, sb_logsunit),	 0 },
>     { offsetof(xfs_sb_t, sb_features2),	 0 },
> -    { sizeof(xfs_sb_t),			 0 }
> +    { offsetof(xfs_sb_t, sb_features2) + sizeof(__uint32_t), 0 }
> };

Whenever you add to the table, you now need to modify both the new
entry and the terminator to get it right.

Nor (IMO) is it obvious that it is a terminator or why it is
different to all the other entries in the structure. A field such as
sb_dummy or sb_pad before the terminator is fairly obvious, and it
means that you don't need to modify the table terminator every time
the superblock gets extended.

That way the code stays more consistent over time, diffs are smaller
and neater, and you can see at a simple diff just how the features
have been added over time (like I did this morning).....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group