Re: [PATCH 30/32] union mount: ext2 fallthru support

[Jan Blunck]

On Tue, May 19, Andreas Dilger wrote:

> On May 19, 2009  11:42 +0200, Jan Blunck wrote:
> > On Mon, May 18, Andreas Dilger wrote:
> > > For enums like this that require specific constant on-disk values
> > > I prefer being safe:
> > > 
> > > enum {
> > > 	EXT2_FT_UNKNOWN  = 0,
> > > 	EXT2_FT_REG_FILE = 1,
> > > 	EXT2_FT_DIR	 = 2,
> > > 	EXT2_FT_CHRDEV   = 3,
> > > 	EXT2_FT_BLKDEV   = 4,
> > > 	EXT2_FT_FIFO     = 5,
> > > 	EXT2_FT_SOCK     = 6,
> > > 	EXT2_FT_SYMLINK  = 7,
> > > 	EXT2_FT_WHT      = 8,
> > > 	EXT2_FT_FALLTHRU = 9,
> > >  	EXT2_FT_MAX
> > > 
> > > 
> > > It probably also makes sense to include a patch for ext3/ext4 to ensure
> > > these values are not used by some unrelated feature.
> > 
> > In ext3 these are preprocessor defines. IIRC defines and enums are identical
> > for C (both an int) so I leave this untouched and just add the new filetypes,
> > right?
> 
> The problem is - what happens if, for whatever reason, EXT2_FT_WHT is
> removed?  In a regular enum EXT2_ET_FALLTHRU would get the old value for
> EXT2_FT_WHT (=8).  Alternately, if someone accidentally adds a value
> before EXT2_FT_WHT because this isn't in the upstream e2fsprogs[*] and
> this would push the values of EXT2_FT_WHT and EXT2_FT_FALLTHROUGH up.
> 
> That is why, when using enums for on-disk or "external" interfaces, I
> prefer that the values are explicitly specified.  It also makes it more
> clear when reading the code that these values are static and should not
> be changed, instead of just a grouping of related constants.

Yes. I totally understand your concerns. I submitted a patch already. I just
wanted to make clear that the way ext3 and ext4 define the filetypes by
preprocessor defines. The defines do not have the problem since they are
explicitly assigning numbers as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/