[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120620044543.GA11330@thor.bakeyournoodle.com>
Date: Wed, 20 Jun 2012 14:45:43 +1000
From: Tony Breeds <tony@...eyournoodle.com>
To: Andreas Dilger <adilger@...ger.ca>
Cc: Ted Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: Minimal configuration for e2fsprogs
On Tue, Jun 19, 2012 at 12:01:54AM -0600, Andreas Dilger wrote:
Thanks for the feedback.
> That is what I would do, though perhaps with macro names that are not
> so confusingly similar to the actual macro names, which might otherwise
> cause confusion if someone doesn't read the code very closely. Like:
>
> #ifdef ENABLE_MMP
> ...
> #define EXT2_LIB_INCOMPAT_SUPP_MMP EXT4_FEATURE_INCOMPAT_MMP
> #else
> #define EXT2_LIB_INCOMPAT_SUPP_MMP (0)
> #endif
Okay, since Ted doesn't mind I went with:
#ifdef CONFIG_MMP
#define EXT4_LIB_INCOMPAT_MMP EXT2_FEATURE_INCOMPAT_MMP
#else
#define EXT4_LIB_INCOMPAT_MMP (0)
#endif
Of course we /could/ #undef them once we're done to be really clear
that they're special.
> This should at least print a message like "This version of debugfs
> does not support MMP. Recompile with --enable-mmp if necessary."
Okay done.
> Having conditionals inline in the code is frowned upon. Better to
> put the conditionals inside ext2fs_mmp_stop(), or declare a no-op
> inline function and keep the rest of the code clean.
Okay I went with putting the conditionals in the ext2fs_mmp_*()
functions. As this removes the changes to ext2fs.h and
lib/ext2fs/Makefile.in
> These should all conditionally become static inline no-op functions
> here (returning zero as needed) so that the calling code does not need
> messy #ifdefs everywhere.
Well here you and Ted disagree :) I'm happy with either. For the time
being I've gone with Ted's idea. I'll let you guys decide on the
relative merits of each approach and then go with whichever "wins"
I'll include by v3 patch in my reply to Ted.
Yours Tony
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists