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@xxxxxxxxxxxxxxxxxx>
Date: Wed, 20 Jun 2012 15:04:43 +1000
Subject: [PATCH] WiP: Add --disable-mmp
Signed-off-by: Tony Breeds <tony@xxxxxxxxxxxxxxxxxx>
---
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
Attachment:
pgpHzYqIIU8nW.pgp
Description: PGP signature
