On Thu, Aug 13, 2009 at 06:03:44PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> ufs1 and ufs2 mainly differ by some structure definitions. Putting
> them into the same module and checking on runtime creates continuous
> if ( ... == UFS1) { ... }
> By using preprocessor it's possible to avoid most of such ifs
> Additionally user needs only one FS in core so we save some space
> core.img with ufs.mod: 23793
> core.img with ufs1.mod: 23078
> core.img with ufs2.mod: 23322Very nice. > +#ifdef MODE_UFS2 > +#define INODE_BLKSZ 8 > +#else > +#define INODE_BLKSZ 4 > +#endif > +#ifndef MODE_UFS2 > +#define UFS_INODE_PER_BLOCK 4 > +#else > +#define UFS_INODE_PER_BLOCK 2 > +#endif When you commit this, could you please follow logical order with ifdef/else/endif? The negation is less intuitive to read. > +#ifdef MODE_UFS2 > + grub_uint64_t indir[UFS_BLKSZ (sblock) / sizeof (grub_uint64_t)]; > +#else > + grub_uint32_t indir[UFS_BLKSZ (sblock) / sizeof (grub_uint32_t)]; > +#endif Can this be made simpler by using typeof() ? (same for the other one below) > - return (data->ufs_type == UFS1) ? indir[blk] : indir[blk << 1]; > + return indir[blk]; The blk bitshift was accounted for elsewhere? (Btw I assume you've tested on both filesystem types). > - ? dirent.namelen_bsd : grub_le_to_cpu16 (dirent.namelen); > +#ifdef MODE_UFS2 > + namelen = dirent.namelen_bsd; > +#else > + namelen = grub_le_to_cpu16 (dirent.namelen); > +#endif I wonder if there was a bug here (native endianess assumed for namelen_bsd?) -- Robert Millan The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all." _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
