diff mbox

Minimal configuration for e2fsprogs

Message ID 20120620052604.GB11330@thor.bakeyournoodle.com
State Superseded, archived
Headers show

Commit Message

Tony Breeds June 20, 2012, 5:26 a.m. UTC 
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@bakeyournoodle.com>
Date: Wed, 20 Jun 2012 15:04:43 +1000
Subject: [PATCH] WiP: Add --disable-mmp

Signed-off-by: Tony Breeds <tony@bakeyournoodle.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(-)

Comments

Tony Breeds June 20, 2012, 5:33 a.m. UTC | #1 
On Wed, Jun 20, 2012 at 03:26:05PM +1000, Tony Breeds wrote:

> 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

Eeek this should be #ifdef oops.

We could also do:
void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
{
#ifndef CONFIG_MMP
	fprintf(.....);
	return;
#endif
	....
}

to avoid the #else and make the approach more consistent with the rest
of the patch.


Yours Tony
Andreas Dilger June 20, 2012, 2:14 p.m. UTC | #2 
On 2012-06-19, at 11:26 PM, Tony Breeds wrote:
> From 5f6a6528c93ad9b3eb878afbeab10c01802647b0 Mon Sep 17 00:00:00 2001
> From: Tony Breeds <tony@bakeyournoodle.com>
> Date: Wed, 20 Jun 2012 15:04:43 +1000
> Subject: [PATCH] WiP: Add --disable-mmp
> 
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.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/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;

I suspect that this will cause warnings from code analysis tools about
dead code, and bloat the code if the compiler isn't optimizing heavily.
Better to put the whole function body inside "#ifdef CONFIG_MMP" and
the "return EXT2_ET_OP_NOT_SUPPORTED" in the #else clause.

Cheers, Andreas
diff mbox

Patch

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;