Re: [PATCH] dinode endianess annotations

[David Chinner <dgc@sgi.com>]

On Fri, Jul 20, 2007 at 06:30:26PM +0200, Christoph Hellwig wrote:
> Biggest bit is duplicating the dinode structure so we have one annoted
> for native endianess and one for disk endinaess.  The other significant
> change is that xfs_xlate_dinode_core is split into one helper per
> direction to allow for proper annotations, everything else is trivial.

Ok, we've needed to do that for a while....

> As a sidenode splitting out the incore dinode means we can move it into
> xfs_inode.h in a later patch and severly improving on the include hell
> in xfs.

Excellent!

Couple of comments, though....

> Index: linux-2.6-xfs/fs/xfs/xfs_dinode.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_dinode.h	2007-07-17 07:54:01.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_dinode.h	2007-07-17 07:54:35.000000000 +0200
> @@ -33,16 +33,17 @@ struct xfs_mount;
>   * with the last field expanding.  It is split into the core and "other"
>   * because we only need the core part in the in-core inode.
>   */
> -typedef struct xfs_timestamp {
> +
> +typedef struct xfs_timestamp_incore {
>  	__int32_t	t_sec;		/* timestamp seconds */
>  	__int32_t	t_nsec;		/* timestamp nanoseconds */
> -} xfs_timestamp_t;
> +} xfs_timestamp_incore_t;

This is a bit ugly - we typically use the "ic" prefix to indicate
"in-core" e.g. "l_iclogbufs", "xfs_icsb_cnts_t".

Perhaps "xfs_ictimestamp_t" or "xfs_ictstamp_t" would be more
consistent with other code. Not sure it really matters, but it would
fix up some of the formatting issues such a long variable
introduces....

>  /*
> - * Note: Coordinate changes to this structure with the XFS_DI_* #defines
> - * below and the offsets table in xfs_ialloc_log_di().
> + * Incore dinode core.  Must match xfs_dinode_core except for endianess
> + * annotations.
>   */
> -typedef struct xfs_dinode_core
> +typedef struct xfs_dinode_incore

So now we have:

	- struct inode (linux in memory inode)
	- struct xfs_inode (xfs in memory inode)
	- struct xfs_dinode_incore (xfs in memory disk inode core)
	- struct xfs_dinode_core (xfs on disk inode core)

I think the xfs_dinode_incore is badly named, because the "core"
part of xfs_dinode_core refers to the main part (or "core") of the
on disk inode. i.e. the non-literal area of the inode. IOWs, "core"
in this context has very different meaning to "incore" (see
XFS_ILOG_CORE and friends, for example).

Perhaps "struct xfs_icdinode_core" as per the above?

Cheers,

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