On Mon, Jul 11, 2011 at 05:32:40PM -0500, Alex Elder wrote:
> - It looks like the three files are:
> - external interface (just function prototypes)
> - internal interface (just function prototypes)
> - structures and accessors, other types, and constants
> Did it just happen to turn out that that the two interface
> files had nothing but prototypes? Or was that what you
> intended to do?
It's more or less intentional. If actually had non-opaque data types
that weren't part of the disk format we might have them here, but the
dir2 code doesn't have any of those.
> - The contents of "xfs_dir2_format.h" comprise more than
> just the on-disk format, it really seems to capture all
> substantive data types and definitions related to directory
> structures in XFS.
The only type not part of the disk format is xfs_dir2_data_aoff_t,
which is a type that we probably should get rid of anyway.
> - None of the dir2 header files themselves #included
> anything else. The same is true for your (new) three
> header files. However, the internal interface file
> (xfs_dir2_priv.h) seems to *always* require the data
> types file (xfs_dir2_format.h) to be included first.
> What are your thoughts about just putting the #include
> of "xfs_dir2_format.h" into "xfs_dir2_priv.h". I
> realize that's really a philosophical question, broader
> than this particular case.
We haven't done that for any of the XFS headers, so I don't
plan to change it with this patch.
> > +#define XFS_DIR2_BLOCK_MAGIC 0x58443242 /* XD2B: for one block dirs */
> /* XD2B: for single block dirs */
>
> > +#define XFS_DIR2_DATA_MAGIC 0x58443244 /* XD2D: for multiblock dirs */
> > +#define XFS_DIR2_FREE_MAGIC 0x58443246 /* XD2F */
> /* XD2F: for free index blocks */
These lines were taken as-is from the old headers, but your variants
are beter, I'll change it.
> > +typedef union {
> > + xfs_dir2_ino8_t i8;
> > + xfs_dir2_ino4_t i4;
> > +} xfs_dir2_inou_t;
> > +#define XFS_DIR2_MAX_SHORT_INUM ((xfs_ino_t)0xffffffffULL)
>
> I know this is historical, but I don't like the use of
> "SHORT" here to mean "4-byte," since "short" in the
> context of directories has a very different meaning
> (i.e., shortform).
We can fix this up later, but I don't want to change identifiers in
addition to the header consolidation.
>
> > +
> > +/*
> > + * Directory layout when stored internal to an inode.
> > + *
> > + * Small directories are packed as tightly as possible so as to fit into the
> > + * literal area of the inode. They consist of a single xfs_dir2_sf_hdr header
> ...of the inode. These "shortform" directories consist...
Ok.
> > + * followed by zero or more xfs_dir2_sf_entry structures. Due the different
> > + * inode number storage size and the variable length name field in
> > + * the xfs_dir2_sf_entry all these structure are variable length, and the
> > + * accessors in this file should be used to iterate over them.
> > + *
> > + *
> > + * The parent directory has a dedicated field, and the self-pointer must
> > + * be calculated on the fly.
>
> This sentence is not very meaningful standing by itself here.
> I think it either needs a bit more context, or maybe it can
> just be removed.
It doesn't seem overly useful, so I'll remove it.
>
> > + *
> > + * Entries are packed toward the top as tightly as possible, and thus may
> > + * be misaligned. Care needs to be taken to access them through special
> > + * helpers or copy them into aligned variables first.
>
> Can this last sentence just be deleted, since above you
> now say that the accessors here should be used?
I'll remove it.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
