Minimal configuration for e2fsprogs

On Mon, Jun 18, 2012 at 01:12:57PM -0400, Ted Ts'o wrote:
> On Mon, Jun 18, 2012 at 03:58:42PM +1000, Tony Breeds wrote:
> > 
> > I'm not certain we're on the same page.  We're linking statically so
> > that fact we don't call the progress functions doesn't matter.  The
> > code is in libext2fs.a and there must be a call path from (eg)
> > ext2fs_open() to fwrite(stderr, ...).  The fact we don't add the 
> > EXT2_FLAG_PRINT_PROGRESS doesn't come into it does it?
> 
> Oh, I see, the problem is that ext2fs_closefs() is calling the
> progress functions; I had forgotten about it.  Yeah, that's something
> I can fix so that the progress functions are only dragged in if mke2fs
> explicitly requests it, via adding function which enables the progress
> functions, instead of just setting the EXT2_FLAG_PRINT_PROGRESS.

Okay that sounds good to me.  If I get the other bits implemented
before you get to it.  I'm happy to take a run at it.
 
> So a couple of things; it's a really bad to define static functions in
> a header file such as ext2fs.h.  Yes, gcc will normally optimize out
> functions which aren't used, but it will also complain with warnings
> if you enable them.

Thanks for the pointers.  I had meant to make them static inline I just
forgot the inline.
 
> Instead, what I would do is to simply take out the calls to the
> ext2fs_mmp library functions in e2fsck and tune2fs, and remove the MMP
> feature flag from the list of supported features.  That way, if
> someone tries to use the e2fsck binary compiled with --disable-mmp on
> a file system with MMP enabled, e2fsck will refuse to touch the file
> system saying that it doesn't support the feature.  If you just make
> the mmp functions silently succeed (by returning 0), that's really bad
> since file system damage could result.

Okay that all make sense I've had a try but I think it's pretty brutal.
I'm sure I could finesse it a bit but I have a couple of questions:

1) The only way I could see to remove MMP from the supported features
was with something like:

#ifdef CONFIG_MMP
#define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...|EXT4_FEATURE_INCOMPAT_MMP|...)
#else
#define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...)
#endif

But EXT2_LIB_FEATURE_INCOMPAT_SUPP is already #defined twice depending
on the state of ENABLE_COMPRESSION.  So going down the path above would
mean #defining it 4 times which I'd think is starting to indicate there
must be a better way.  Firstly am I in the right ball park, secondly
Would something like

#ifdef ENABLE_COMPRESSION
...
#define _EXT4_FEATURE_INCOMPAT_COMPRESSION EXT4_FEATURE_INCOMPAT_COMPRESSION
#else
#define _EXT4_FEATURE_INCOMPAT_COMPRESSION (0)
#endif

#ifdef ENABLE_MMP
...
#define _EXT4_FEATURE_INCOMPAT_MMP EXT4_FEATURE_INCOMPAT_MMP
#else
#define _EXT4_FEATURE_INCOMPAT_MMP (0)
#endif

#define EXT2_LIB_FEATURE_INCOMPAT_SUPP  (...\
                                         _EXT2_FEATURE_INCOMPAT_COMPRESSION|\
                                         ... \
                                         _EXT4_FEATURE_INCOMPAT_MMP|\
                                         ...)

Be acceptable?

2) in debugfs you have a command table (debug_cmds.ct or similar) there
doesn't seem to be a way to exclude commands based on the state of
CONFIG_MMP.  Is it a problem the expose a debugfs command that will do
nothing when built with --disable-mmp, or should I look at extending
the command table generator to support the conditionals?

I'll add my current patch after my .signature

> I already have things set up so that if you don't actually call the
> functions to read in the bitmaps, the functions to read and write the
> bitmaps don't get dragged into a static library link.  That's what I'm
> referring to.

Ahh thanks for the clarification.
 
> The functions to actually create in-memory bitmaps for various
> housekeeping things, do need to get dragged in, but we don't
> necessarily need to drag in all of the different bitmap back-ends.  If
> we assume that yaboot doesn't need to optimize for low-memory usage
> for really, really big file systems (i.e., you're not going to try to
> allocate files or blocks while opening a multi-terrabyte file system
> using libext2fs on a system with only 512mb), then you wouldn't need
> blkmap64_rb.c.

Right, we're operating in a pretty tight memory environment (typically
128MB or 256MB) but we're also usually opening file-systems <100GB
usually closer to 200MB.
 
> And the bitmap statistics functions are sometihng we can use a
> configure option to disable, which should remove the last bits of
> stdio usage I believe.

Okay.  If I can get the disable-mmp patch into a state you like it I'll
tackle that configure option as well.

> If you'd like to try to clean up the --disable-mmp patch, and perhaps
> try your hand at a --disable-libext2fs-stats patch which disable the
> statistics options, that would be great.  Otherwise, I'll try to get
> to it in the next few weeks.

I really appreciate you help and time, and hope that continually
pointing me in the right direction isn't too taxing on you.

Yours Tony

---
 configure.in           |   21 +++++++++++++++++++++
 debugfs/debugfs.c      |    2 ++
 debugfs/set_fields.c   |    9 +++++++++
 e2fsck/journal.c       |    2 ++
 e2fsck/unix.c          |    4 ++++
 e2fsck/util.c          |    6 ++++++
 lib/config.h.in        |    3 +++
 lib/ext2fs/Makefile.in |    4 ++--
 lib/ext2fs/closefs.c   |    2 ++
 lib/ext2fs/ext2fs.h    |    2 ++
 lib/ext2fs/openfs.c    |    2 ++
 misc/mke2fs.c          |    2 ++
 misc/tune2fs.c         |    6 ++++++
 13 files changed, 63 insertions(+), 2 deletions(-)

On 2012-06-18, at 11:48 PM, Tony Breeds wrote:
> Okay that all make sense I've had a try but I think it's pretty brutal.
> I'm sure I could finesse it a bit but I have a couple of questions:
> 
> 1) The only way I could see to remove MMP from the supported features
> was with something like:
> 
> #ifdef CONFIG_MMP
> #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...|EXT4_FEATURE_INCOMPAT_MMP|...)
> #else
> #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...)
> #endif
> 
> But EXT2_LIB_FEATURE_INCOMPAT_SUPP is already #defined twice depending
> on the state of ENABLE_COMPRESSION.  So going down the path above would
> mean #defining it 4 times which I'd think is starting to indicate there
> must be a better way.

That is definitely NOT the way to go.

> Firstly am I in the right ball park, secondly Would something like
> 
> #ifdef ENABLE_COMPRESSION
> ...
> #define _EXT4_FEATURE_INCOMPAT_COMPRESSION EXT4_FEATURE_INCOMPAT_COMPRESSION
> #else
> #define _EXT4_FEATURE_INCOMPAT_COMPRESSION (0)
> #endif
> 
> #ifdef ENABLE_MMP
> ...
> #define _EXT4_FEATURE_INCOMPAT_MMP EXT4_FEATURE_INCOMPAT_MMP
> #else
> #define _EXT4_FEATURE_INCOMPAT_MMP (0)
> #endif
> 
> #define EXT2_LIB_FEATURE_INCOMPAT_SUPP  (...\
>                                         _EXT2_FEATURE_INCOMPAT_COMPRESSION|\
>                                         ... \
>                                         _EXT4_FEATURE_INCOMPAT_MMP|\
>                                         ...)
> 
> Be acceptable?

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

> 2) in debugfs you have a command table (debug_cmds.ct or similar) there
> doesn't seem to be a way to exclude commands based on the state of
> CONFIG_MMP.  Is it a problem the expose a debugfs command that will do
> nothing when built with --disable-mmp, or should I look at extending
> the command table generator to support the conditionals?
> 
> I'll add my current patch after my .signature
> 
>> I already have things set up so that if you don't actually call the
>> functions to read in the bitmaps, the functions to read and write the
>> bitmaps don't get dragged into a static library link.  That's what I'm
>> referring to.
> 
> Ahh thanks for the clarification.
> 
>> The functions to actually create in-memory bitmaps for various
>> housekeeping things, do need to get dragged in, but we don't
>> necessarily need to drag in all of the different bitmap back-ends.  If
>> we assume that yaboot doesn't need to optimize for low-memory usage
>> for really, really big file systems (i.e., you're not going to try to
>> allocate files or blocks while opening a multi-terrabyte file system
>> using libext2fs on a system with only 512mb), then you wouldn't need
>> blkmap64_rb.c.
> 
> Right, we're operating in a pretty tight memory environment (typically
> 128MB or 256MB) but we're also usually opening file-systems <100GB
> usually closer to 200MB.
> 
>> And the bitmap statistics functions are sometihng we can use a
>> configure option to disable, which should remove the last bits of
>> stdio usage I believe.
> 
> Okay.  If I can get the disable-mmp patch into a state you like it I'll
> tackle that configure option as well.
> 
>> If you'd like to try to clean up the --disable-mmp patch, and perhaps
>> try your hand at a --disable-libext2fs-stats patch which disable the
>> statistics options, that would be great.  Otherwise, I'll try to get
>> to it in the next few weeks.
> 
> I really appreciate you help and time, and hope that continually
> pointing me in the right direction isn't too taxing on you.
> 
> Yours Tony
> 
> ---
> configure.in           |   21 +++++++++++++++++++++
> debugfs/debugfs.c      |    2 ++
> debugfs/set_fields.c   |    9 +++++++++
> e2fsck/journal.c       |    2 ++
> e2fsck/unix.c          |    4 ++++
> e2fsck/util.c          |    6 ++++++
> lib/config.h.in        |    3 +++
> lib/ext2fs/Makefile.in |    4 ++--
> lib/ext2fs/closefs.c   |    2 ++
> lib/ext2fs/ext2fs.h    |    2 ++
> lib/ext2fs/openfs.c    |    2 ++
> misc/mke2fs.c          |    2 ++
> misc/tune2fs.c         |    6 ++++++
> 13 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index 7373e8e..4bacd1a 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -746,6 +746,27 @@ AC_MSG_RESULT([Building uuidd by default])
> )
> AC_SUBST(UUIDD_CMT)
> dnl
> +dnl handle --disable-mmp
> +dnl
> +AH_TEMPLATE([CONFIG_MMP], [Define to 1 to enable mmp support])
> +AC_ARG_ENABLE([mmp],
> +[  --disable-mmp   	  disable support mmp, Multi Mount Protection],
> +if test "$enableval" = "no"
> +then
> +	AC_MSG_RESULT([Disabling mmp support])
> +	MMP_CMT="#"
> +else
> +	AC_MSG_RESULT([Enabling mmp support])
> +	AC_DEFINE(CONFIG_MMP, 1)
> +	MMP_CMT=
> +fi
> +,
> +AC_MSG_RESULT([Enabling mmp support by default])
> +AC_DEFINE(CONFIG_MMP, 1)
> +MMP_CMT=
> +)
> +AC_SUBST(MMP_CMT)
> +dnl
> dnl
> dnl
> MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index cf80bd0..5df915e 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -2194,6 +2194,7 @@ void do_punch(int argc, char *argv[])
> 
> void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
> {
> +#ifdef CONFIG_MMP
> 	struct ext2_super_block *sb;
> 	struct mmp_struct *mmp_s;
> 	time_t t;
> @@ -2237,6 +2238,7 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
> 	fprintf(stdout, "node_name: %s\n", mmp_s->mmp_nodename);
> 	fprintf(stdout, "device_name: %s\n", mmp_s->mmp_bdevname);
> 	fprintf(stdout, "magic: 0x%x\n", mmp_s->mmp_magic);
> +#endif

This should at least print a message like "This version of debugfs
does not support MMP.  Recompile with --enable-mmp if necessary."

> }
> 
> static int source_file(const char *cmd_file, int sci_idx)
> diff --git a/debugfs/set_fields.c b/debugfs/set_fields.c
> index 08bfd8d..77fbbc1 100644
> --- a/debugfs/set_fields.c
> +++ b/debugfs/set_fields.c
> @@ -69,8 +69,10 @@ static errcode_t parse_hashalg(struct field_set_info *info, char *field, char *a
> static errcode_t parse_time(struct field_set_info *info, char *field, char *arg);
> static errcode_t parse_bmap(struct field_set_info *info, char *field, char *arg);
> static errcode_t parse_gd_csum(struct field_set_info *info, char *field, char *arg);
> +#ifdef CONFIG_MMP
> static errcode_t parse_mmp_clear(struct field_set_info *info, char *field,
> 				 char *arg);
> +#endif
> 
> static struct field_set_info super_fields[] = {
> 	{ "inodes_count", &set_sb.s_inodes_count, NULL, 4, parse_uint },
> @@ -246,6 +248,7 @@ static struct field_set_info ext4_bg_fields[] = {
> };
> 
> static struct field_set_info mmp_fields[] = {
> +#ifdef CONFIG_MMP
> 	{ "clear", &set_mmp.mmp_magic, NULL, sizeof(set_mmp), parse_mmp_clear },
> 	{ "magic", &set_mmp.mmp_magic, NULL, 4, parse_uint },
> 	{ "seq", &set_mmp.mmp_seq, NULL, 4, parse_uint },
> @@ -255,6 +258,8 @@ static struct field_set_info mmp_fields[] = {
> 	{ "bdevname", &set_mmp.mmp_bdevname, NULL, sizeof(set_mmp.mmp_bdevname),
> 		parse_string },
> 	{ "check_interval", &set_mmp.mmp_check_interval, NULL, 2, parse_uint },
> +#endif
> +	{ 0, 0, 0, 0 }
> };
> 
> static int check_suffix(const char *field)
> @@ -748,6 +753,7 @@ void do_set_block_group_descriptor(int argc, char *argv[])
> 	}
> }
> 
> +#ifdef CONFIG
> static errcode_t parse_mmp_clear(struct field_set_info *info,
> 				 char *field EXT2FS_ATTR((unused)),
> 				 char *arg EXT2FS_ATTR((unused)))
> @@ -762,9 +768,11 @@ static errcode_t parse_mmp_clear(struct field_set_info *info,
> 
> 	return 1; /* we don't need the MMP block written again */
> }
> +#endif
> 
> void do_set_mmp_value(int argc, char *argv[])
> {
> +#ifdef CONFIG_MMP
> 	const char *usage = "<field> <value>\n"
> 		"\t\"set_mmp_value -l\" will list the names of "
> 		"MMP fields\n\twhich can be set.";
> @@ -819,5 +827,6 @@ void do_set_mmp_value(int argc, char *argv[])
> 				 &set_mmp);
> 		*mmp_s = set_mmp;
> 	}
> +#endif
> }
> 
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index 767ea10..e5ee4ed 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -889,7 +889,9 @@ int e2fsck_run_ext3_journal(e2fsck_t ctx)
> 	if (stats && stats->bytes_written)
> 		kbytes_written = stats->bytes_written >> 10;
> 
> +#ifdef CONFIG_MMP
> 	ext2fs_mmp_stop(ctx->fs);
> +#endif

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.

> 	ext2fs_free(ctx->fs);
> 	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
> 			     ctx->superblock, blocksize, io_ptr,
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 94260bd..6cca9e0 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1042,6 +1042,7 @@ static const char *my_ver_date = E2FSPROGS_DATE;
> 
> static int e2fsck_check_mmp(ext2_filsys fs, e2fsck_t ctx)
> {
> +#ifdef CONFIG_MMP
> 	struct mmp_struct *mmp_s;
> 	unsigned int mmp_check_interval;
> 	errcode_t retval = 0;
> @@ -1120,6 +1121,9 @@ check_error:
> 		}
> 	}
> 	return retval;
> +#else
> +	return 0;
> +#endif
> }
> 
> int main (int argc, char *argv[])
> diff --git a/e2fsck/util.c b/e2fsck/util.c
> index 7c4caab..5b9b154 100644
> --- a/e2fsck/util.c
> +++ b/e2fsck/util.c
> @@ -56,7 +56,9 @@ void fatal_error(e2fsck_t ctx, const char *msg)
> 	if (!fs)
> 		goto out;
> 	if (fs->io) {
> +#ifdef CONFIG_MMP
> 		ext2fs_mmp_stop(ctx->fs);
> +#endif
> 		if (ctx->fs->io->magic == EXT2_ET_MAGIC_IO_CHANNEL)
> 			io_channel_flush(ctx->fs->io);
> 		else
> @@ -780,6 +782,7 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
> 
> errcode_t e2fsck_mmp_update(ext2_filsys fs)
> {
> +#ifdef CONFIF_MMP
> 	errcode_t retval;
> 
> 	retval = ext2fs_mmp_update(fs);
> @@ -789,6 +792,9 @@ errcode_t e2fsck_mmp_update(ext2_filsys fs)
> 			       "being modified while fsck is running.\n"));
> 
> 	return retval;
> +#else
> +	return 0;
> +#endif
> }
> 
> void e2fsck_set_bitmap_type(ext2_filsys fs, unsigned int default_type,
> diff --git a/lib/config.h.in b/lib/config.h.in
> index 90e9743..52c3897 100644
> --- a/lib/config.h.in
> +++ b/lib/config.h.in
> @@ -12,6 +12,9 @@
> /* Define to 1 if debugging ext3/4 journal code */
> #undef CONFIG_JBD_DEBUG
> 
> +/* Define to 1 to enable mmp support */
> +#undef CONFIG_MMP
> +
> /* Define to 1 to enable quota support */
> #undef CONFIG_QUOTA
> 
> diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
> index 0d9ac21..5b73958 100644
> --- a/lib/ext2fs/Makefile.in
> +++ b/lib/ext2fs/Makefile.in
> @@ -14,9 +14,10 @@ MK_CMDS=	_SS_DIR_OVERRIDE=../ss ../ss/mk_cmds
> @RESIZER_CMT@RESIZE_LIB_OBJS = dupfs.o
> @TEST_IO_CMT@TEST_IO_LIB_OBJS = test_io.o
> @IMAGER_CMT@E2IMAGE_LIB_OBJS = imager.o
> +@MMP_CMT@MMP_OBJS = mmp.o
> 
> OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
> -	$(TEST_IO_LIB_OBJS) \
> +	$(TEST_IO_LIB_OBJS) $(MMP_OBJS) \
> 	ext2_err.o \
> 	alloc.o \
> 	alloc_sb.o \
> @@ -66,7 +67,6 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
> 	lookup.o \
> 	mkdir.o \
> 	mkjournal.o \
> -	mmp.o \
> 	namei.o \
> 	native.o \
> 	newdir.o \
> diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
> index 973c2a2..abf77cc 100644
> --- a/lib/ext2fs/closefs.c
> +++ b/lib/ext2fs/closefs.c
> @@ -464,9 +464,11 @@ errcode_t ext2fs_close2(ext2_filsys fs, int flags)
> 			return retval;
> 	}
> 
> +#ifdef CONFIG_MMP
> 	retval = ext2fs_mmp_stop(fs);
> 	if (retval)
> 		return retval;
> +#endif
> 
> 	ext2fs_free(fs);
> 	return 0;
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index ff088bb..fc83973 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -1366,6 +1366,7 @@ errcode_t ext2fs_unlink(ext2_filsys fs, ext2_ino_t dir, const char *name,
> 			ext2_ino_t ino, int flags);
> 
> /* mmp.c */
> +#ifdef CONFIG_MMP
> errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf);
> errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf);
> errcode_t ext2fs_mmp_clear(ext2_filsys fs);
> @@ -1374,6 +1375,7 @@ errcode_t ext2fs_mmp_start(ext2_filsys fs);
> errcode_t ext2fs_mmp_update(ext2_filsys fs);
> errcode_t ext2fs_mmp_stop(ext2_filsys fs);
> unsigned ext2fs_mmp_new_seq(void);
> +#endif /* CONFIG_MMP */

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.

> 
> /* read_bb.c */
> extern errcode_t ext2fs_read_bb_inode(ext2_filsys fs,
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index 482e4ab..88fdd4d 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -391,6 +391,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
> 	fs->flags &= ~EXT2_FLAG_NOFREE_ON_ERROR;
> 	*ret_fs = fs;
> 
> +#ifdef CONFIG_MMP
> 	if ((fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) &&
> 	    !(flags & EXT2_FLAG_SKIP_MMP) &&
> 	    (flags & (EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE))) {
> @@ -401,6 +402,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
> 			goto cleanup;
> 		}
> 	}
> +#endif
> 
> 	return 0;
> cleanup:
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 7ec8cc2..e9b4c1f 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -2557,6 +2557,7 @@ int main (int argc, char *argv[])
> 			printf(_("done\n"));
> 	}
> no_journal:
> +#ifdef CONFIG_MMP
> 	if (!super_only &&
> 	    fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) {
> 		retval = ext2fs_mmp_init(fs);
> @@ -2570,6 +2571,7 @@ no_journal:
> 				 "with update interval %d seconds.\n"),
> 			       fs->super->s_mmp_update_interval);
> 	}
> +#endif
> 
> 	if (EXT2_HAS_RO_COMPAT_FEATURE(&fs_param,
> 				       EXT4_FEATURE_RO_COMPAT_BIGALLOC))
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index f1f0bcf..2c4b20b 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -430,6 +430,7 @@ static int update_feature_set(ext2_filsys fs, char *features)
> 				return 1;
> 		}
> 	}
> +#ifdef CONFIG_MMP
> 	if (FEATURE_ON(E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_MMP)) {
> 		int error;
> 
> @@ -495,6 +496,7 @@ mmp_error:
> 		sb->s_mmp_block = 0;
> 		sb->s_mmp_update_interval = 0;
> 	}
> +#endif
> 
> 	if (FEATURE_ON(E2P_FEATURE_COMPAT, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
> 		/*
> @@ -2105,10 +2107,12 @@ retry_open:
> 			goto closefs;
> 		}
> 	}
> +#ifdef CONFIG_MMP
> 	if (clear_mmp) {

If the code compiles so that "clear_mmp == 0" always, then the compiler
will likely optimize this codepath out.  In any case, ext2fs_mmp_clear()
itself is already a no-op in your case, so this code would only be a
few bytes if it wasn't optimized away completely.

> 		rc = ext2fs_mmp_clear(fs);
> 		goto closefs;
> 	}
> +#endif
> 	if (journal_size || journal_device) {
> 		rc = add_journal(fs);
> 		if (rc)
> @@ -2219,7 +2223,9 @@ retry_open:
> 
> closefs:
> 	if (rc) {
> +#ifdef CONFIG_MMP
> 		ext2fs_mmp_stop(fs);
> +#endif
> 		exit(1);
> 	}
> 
> -- 
> 1.7.6.4
> 


Cheers, Andreas

On Tue, Jun 19, 2012 at 12:01:54AM -0600, Andreas Dilger wrote:
> 
> 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

either name is fine with me; I'm not particularly picky here, since
the only place the #define would get used is the header file, the
opportunities for confusion are pretty limited.

> > 2) in debugfs you have a command table (debug_cmds.ct or similar) there
> > doesn't seem to be a way to exclude commands based on the state of
> > CONFIG_MMP.  Is it a problem the expose a debugfs command that will do
> > nothing when built with --disable-mmp, or should I look at extending
> > the command table generator to support the conditionals?

I'd have it print an error of some kind, i.e., "MMP not supported"


> > --- a/lib/ext2fs/ext2fs.h
> > +++ b/lib/ext2fs/ext2fs.h
> > @@ -1366,6 +1366,7 @@ errcode_t ext2fs_unlink(ext2_filsys fs, ext2_ino_t dir, const char *name,
> > 			ext2_ino_t ino, int flags);
> > 
> > /* mmp.c */
> > +#ifdef CONFIG_MMP
> > errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf);
> > errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf);
> > errcode_t ext2fs_mmp_clear(ext2_filsys fs);
> > @@ -1374,6 +1375,7 @@ errcode_t ext2fs_mmp_start(ext2_filsys fs);
> > errcode_t ext2fs_mmp_update(ext2_filsys fs);
> > errcode_t ext2fs_mmp_stop(ext2_filsys fs);
> > unsigned ext2fs_mmp_new_seq(void);
> > +#endif /* CONFIG_MMP */
> 
> 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.

If we've removed MMP from the list of supported features, a much
simpler thing we can do is to just add at the beginning of each of the
functions in mmp.c:

#ifndef CONFIG_MMP
	return EXT2_ET_OP_NOT_SUPPORTED;
#endif

and then letting the compiler optimize out the rest of the code in the
function; I wouldn't worry about using static inlines, since it's not
something we really need to optimize in this case.  The mmp functions
don't get called all that often anyway.

I also wouldn't bother commenting out the function signatures in
ext2fs.h.  For programs which are including ext2fs.h, they're not
going to be including the config.h which has CONFIG_* or ENABLE_*
autoconf #defines anyway.

Arguably all of the EXT2_LIB_* defines shouldn't be in ext2fs.h for
that reason; they should be in ext2fsP.h, and the only reason why they
aren't is due to history (since we didn't have ext2fsP.h when they
were first introduced).  At some point I'll probably move that logic
into ext2fsP.h, just to keep ext2fs.h smaller/cleaner for application
programs.

Regards,

						- Ted

P.S.  I'm debating whether or not it makes sense to create a special
static library just for bootloaders which is stripped down and
read-only.  I realized while looking at things that there is a pretty
large amount of coding getting dragged in due to namei.c pulling in
dir_iterate.c pulling in block_iterate.c pulling in extent.c, which
then pulls in a lot of code since we might need to allocate or
deallocate blocks.  If we made a read-only variant of the library, it
could significantly reduce the size of the statically linked
bootloader.  I'm not entirely sure it's worth it, since you don't seem
to be worrying about the compiled binary size of yaboot, but it's
something we could do if it was interesting.  What do you think?
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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