[Ocfs2-tools-devel] [PATCH 1/4] Adds o2image header and supporting functions

[Joel Becker]

On Tue, May 06, 2008 at 04:59:33PM -0700, Sunil Mushran wrote:
> Signed-off-by: Sunil Mushran <sunil.mushran at oracle.com>
> 
> Small comment inlined.

Next time, trim some of the context :-)
> > +/* on disk ocfs2 image header format */
> > +struct ocfs2_image_hdr {
> > +	__le32	hdr_magic;
> > +	__le32	hdr_timestamp;		/* Time of image creation */
> > +	__u8	hdr_magic_desc[16];	/* "OCFS2 IMAGE" */
> > +	__le64	hdr_version;		/* ocfs2 image version */
> > +	__le64	hdr_fsblks;		/* blocks in filesystem */
> > +	__le64	hdr_fsblksz;		/* Filesystem block size */
> > +	__le64	hdr_imgblks;		/* Filesystem blocks in image */
> > +	__le64	hdr_blksz;		/* bitmap block size */
> >   
> 
> hdr_fsblks and hdr_fsblksz are ripe for typos. How about being
> more explicit.
> 
> hdr_fsblks ==> hdr_fsblkcnt
> hdr_imgblks ==> hdr_imgblkcnt
> hdr_blksz ==> hdr_bmblksz

	How about hdr_fs_blocks?  We're not on OS/360 here :-)  And I
like 'fsblks' better than 'fsblkcnt', but really, 'fs_blocks' is truly
explicit.
	hdr_fs_blocks, hdr_image_blocks, hdr_blocksize.

> ost_fsblks ==> ost_fsblkcnt
> ost_imgblks ==> ost_imgblkcnt

	ost_fs_blocks, ost_image_blocks.

Joel

-- 

 "I'm living so far beyond my income that we may almost be said
 to be living apart."
         - e e cummings

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127