Message-ID: <20120620052604.GB11330@thor.bakeyournoodle.com>
Date:	Wed, 20 Jun 2012 15:26:05 +1000
From:	Tony Breeds <tony@...eyournoodle.com>
To:	Ted Ts'o <tytso@....edu>
Cc:	Andreas Dilger <adilger@...ger.ca>, linux-ext4@...r.kernel.org
Subject: Re: Minimal configuration for e2fsprogs

On Tue, Jun 19, 2012 at 09:56:10AM -0400, Ted Ts'o wrote:
> 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.

Okay.  As I said to Andreas we could #undef them once we're done with
them.
 
> > > 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"

Okay done.

<snip>

> 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.

Okay all taken on board.
 
> 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?

Well that does seem to be a lot of work.  Yaboot isn't really too worried
about code size and I worry that yaboot's requirements from this library
would probably be different to another boot loaders.

For example I know that grub2 can boot from a Fibre Channel device so
right off the bat MMP might be something they want.

I guess that means it's something that needs a reasonable amount of
thought.

Below is my "v3" of the patch.  It's only been tested by building
./configure && make install install-libs  && (cd ~/yaboot && make)
./configure --enable-mmp && make install install-libs  && (cd ~/yaboot && make)
./configure --disable-mmp && make install install-libs  && (cd ~/yaboot && make)

and the yaboot link behaves as expected.

From my point of view I'd need to submit the changes in behavior of
ENABLE_COMPRESSION as one patch so that we stick with the one patch one
logical change approach.  For the sake of review/feedback it's a single
patch.

Yours Tony


From 5f6a6528c93ad9b3eb878afbeab10c01802647b0 Mon Sep 17 00:00:00 2001
From: Tony Breeds <tony@...eyournoodle.com>
Date: Wed, 20 Jun 2012 15:04:43 +1000
Subject: [PATCH] WiP: Add --disable-mmp

Signed-off-by: Tony Breeds <tony@...eyournoodle.com>
---
 configure.in         |   17 +++++++++++++++++
 debugfs/debugfs.c    |    5 +++++
 debugfs/set_fields.c |    5 +++++
 lib/config.h.in      |    3 +++
 lib/ext2fs/ext2fs.h  |   23 ++++++++++++-----------
 lib/ext2fs/mmp.c     |   24 ++++++++++++++++++++++++
 6 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/configure.in b/configure.in
index 7373e8e..a02234d 100644
--- a/configure.in
+++ b/configure.in
@@ -746,6 +746,23 @@ 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])
+else
+	AC_MSG_RESULT([Enabling mmp support])
+	AC_DEFINE(CONFIG_MMP, 1)
+fi
+,
+AC_MSG_RESULT([Enabling mmp support by default])
+AC_DEFINE(CONFIG_MMP, 1)
+)
+dnl
 dnl
 dnl
 MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library
diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index cf80bd0..0c27b1e 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[])
 {
+#if CONFIG_MMP
 	struct ext2_super_block *sb;
 	struct mmp_struct *mmp_s;
 	time_t t;
@@ -2237,6 +2238,10 @@ 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);
+#else
+	fprintf(stdout, "MMP is unsupported, please recompile with "
+	                "--enable-mmp\n");
+#endif
 }
 
 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..a42faa2 100644
--- a/debugfs/set_fields.c
+++ b/debugfs/set_fields.c
@@ -765,6 +765,7 @@ static errcode_t parse_mmp_clear(struct field_set_info *info,
 
 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 +820,9 @@ void do_set_mmp_value(int argc, char *argv[])
 				 &set_mmp);
 		*mmp_s = set_mmp;
 	}
+#else
+	fprintf(stdout, "MMP is unsupported, please recompile with "
+	                "--enable-mmp\n");
+#endif
 }
 
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/ext2fs.h b/lib/ext2fs/ext2fs.h
index ff088bb..542b20f 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -554,25 +554,26 @@ typedef struct ext2_icount *ext2_icount_t;
    environment at configure time. */
  #warning "Compression support is experimental"
 #endif
-#define EXT2_LIB_FEATURE_INCOMPAT_SUPP	(EXT2_FEATURE_INCOMPAT_FILETYPE|\
-					 EXT2_FEATURE_INCOMPAT_COMPRESSION|\
-					 EXT3_FEATURE_INCOMPAT_JOURNAL_DEV|\
-					 EXT2_FEATURE_INCOMPAT_META_BG|\
-					 EXT3_FEATURE_INCOMPAT_RECOVER|\
-					 EXT3_FEATURE_INCOMPAT_EXTENTS|\
-					 EXT4_FEATURE_INCOMPAT_FLEX_BG|\
-					 EXT4_FEATURE_INCOMPAT_MMP|\
-					 EXT4_FEATURE_INCOMPAT_64BIT)
+#define EXT2_LIB_INCOMPAT_COMPRESSION	EXT2_FEATURE_INCOMPAT_COMPRESSION
 #else
+#define EXT2_LIB_INCOMPAT_COMPRESSION	(0)
+#endif
+
+#ifdef CONFIG_MMP
+#define EXT4_LIB_INCOMPAT_MMP		EXT4_FEATURE_INCOMPAT_MMP
+#else
+#define EXT4_LIB_INCOMPAT_MMP		(0)
+#endif
+
 #define EXT2_LIB_FEATURE_INCOMPAT_SUPP	(EXT2_FEATURE_INCOMPAT_FILETYPE|\
+					 EXT2_LIB_INCOMPAT_COMPRESSION|\
 					 EXT3_FEATURE_INCOMPAT_JOURNAL_DEV|\
 					 EXT2_FEATURE_INCOMPAT_META_BG|\
 					 EXT3_FEATURE_INCOMPAT_RECOVER|\
 					 EXT3_FEATURE_INCOMPAT_EXTENTS|\
 					 EXT4_FEATURE_INCOMPAT_FLEX_BG|\
-					 EXT4_FEATURE_INCOMPAT_MMP|\
+					 EXT4_LIB_INCOMPAT_MMP|\
 					 EXT4_FEATURE_INCOMPAT_64BIT)
-#endif
 #ifdef CONFIG_QUOTA
 #define EXT2_LIB_FEATURE_RO_COMPAT_SUPP	(EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER|\
 					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE|\
diff --git a/lib/ext2fs/mmp.c b/lib/ext2fs/mmp.c
index bb3772d..a783698 100644
--- a/lib/ext2fs/mmp.c
+++ b/lib/ext2fs/mmp.c
@@ -33,6 +33,9 @@
 
 errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct mmp_struct *mmp_cmp;
 	errcode_t retval = 0;
 
@@ -92,6 +95,9 @@ out:
 
 errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct mmp_struct *mmp_s = buf;
 	struct timeval tv;
 	errcode_t retval = 0;
@@ -128,6 +134,9 @@ errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf)
 
 unsigned ext2fs_mmp_new_seq()
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	unsigned new_seq;
 	struct timeval tv;
 
@@ -182,6 +191,9 @@ out:
 
 errcode_t ext2fs_mmp_clear(ext2_filsys fs)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	errcode_t retval = 0;
 
 	if (!(fs->flags & EXT2_FLAG_RW))
@@ -194,6 +206,9 @@ errcode_t ext2fs_mmp_clear(ext2_filsys fs)
 
 errcode_t ext2fs_mmp_init(ext2_filsys fs)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct ext2_super_block *sb = fs->super;
 	blk64_t mmp_block;
 	errcode_t retval;
@@ -229,6 +244,9 @@ out:
  */
 errcode_t ext2fs_mmp_start(ext2_filsys fs)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct mmp_struct *mmp_s;
 	unsigned seq;
 	unsigned int mmp_check_interval;
@@ -328,6 +346,9 @@ mmp_error:
  */
 errcode_t ext2fs_mmp_stop(ext2_filsys fs)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct mmp_struct *mmp, *mmp_cmp;
 	errcode_t retval = 0;
 
@@ -366,6 +387,9 @@ mmp_error:
  */
 errcode_t ext2fs_mmp_update(ext2_filsys fs)
 {
+#ifndef CONFIG_MMP
+	return EXT2_ET_OP_NOT_SUPPORTED;
+#endif
 	struct mmp_struct *mmp, *mmp_cmp;
 	struct timeval tv;
 	errcode_t retval = 0;
-- 
1.7.6.4


Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists