Minimal configuration for e2fsprogs

Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 2515 bytes --]

Hi All,
	I appologise if this is not the correct place to discuss this,
or if it's been discussed before.  In either event please point me in
the right direction and I'll move along.

I'm the maintainer for yaboot a bootloader for powerpc systems.  We link
against libext2fs.a, but as we're a bootloader we do NOT link against
libc as such we need to implement a number of "stub" functions to keep
up with the newer features being added here.

Our current set looks like:
---
int printf(const char *format, ...);
int fprintf(FILE *stream, const char *format, ...);
int fputs(const char *s, FILE *stream);
int fflush(FILE *stream);
char *getenv(const char *name);
int gethostname(char *name, size_t len);
int gettimeofday(struct timeval *tv, struct timezone *tz);
int * __errno_location(void);
unsigned int sleep(unsigned int seconds);
int rand(void);
void srand(unsigned int seed);
long int random(void);
void srandom(unsigned int seed);
uid_t geteuid(void);
uid_t getuid(void);
pid_t getpid(void);
int stat(const char *path, struct stat *buf);
int stat64(const char *path, struct stat *buf);
int fstat(int fd, struct stat *buf);
int fstat64(int fd, struct stat *buf);
int open(const char *pathname, int flags, mode_t mode);
int open64(const char *pathname, int flags, mode_t mode);
off_t lseek(int fd, off_t offset, int whence);
off64_t lseek64(int fd, off64_t offset, int whence);
ssize_t read(int fildes, void *buf, size_t nbyte);
int close(int fd);
void *calloc(size_t nmemb, size_t size);
void perror(const char *s);
void exit(int status);
int ioctl(int d, int request, ...);
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
long sysconf(int name);
int getpagesize(void);
void qsort(void *base, size_t nmemb, size_t size,
           int(*compar)(const void *, const void *));
---

Plus a few typdefs and #defines.

It looks to me (with only a quick look) that a lot of these libc
functions are used for nice error messages to the end user.

So my question is essentially twofold.
1) Is there a "good" way to ./configure e2fsprogs to build a really
minimal libext2fs.a ?
2) If not can I get some pointers adding a --really-minimal autoconf
argument to pretty much do what I need.

Of course getting e2fsprogs to support this minimal build is only the
first step I then need to convince the distros to package it.

I did consider, cp'ing the code into yaboot but that seems pretty gross
and would end up breaching the Fedora Packaging Guidelines.

Any advice gladly accepted.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Ted Ts'o]

On Fri, Jun 15, 2012 at 02:24:21PM +1000, Tony Breeds wrote:
> 	I appologise if this is not the correct place to discuss this,
> or if it's been discussed before.  In either event please point me in
> the right direction and I'll move along.
> 
> I'm the maintainer for yaboot a bootloader for powerpc systems.  We link
> against libext2fs.a, but as we're a bootloader we do NOT link against
> libc as such we need to implement a number of "stub" functions to keep
> up with the newer features being added here.

So I think you need to break your list a bit more.  There are a number
of functions on this list where, if yaboot doesn't actually call a
particular function (for example, the progess bar functions, in
lib/ext2fs/progress.c), the fact that they reference the stdio
functions won't matter.  Something similar will be going on with
test_io.c; if you don't use those functions, you won't need to provide
stubs for any of the libc functions called by them.

There are some cases where we may need to create some configure
options.  For example, there are a large number of the symbols that
you have listed that got dragged in by the lib/ext2fs/mmp.c.  That's
one where it may be simplest to add a set of #ifdef's that comment out
multi-mount protection --- I assume you don't plan to support booting
over fibre channel where you might need to care about having two
systems trying to modify the same file system at the same time.  :-)

Which brings up another point --- are you only planning on opening the
file system read-only, or do you expect to modify the file system from
yaboot?  If you don't need to modify the file system, and so you don't
need to load the bitmap manipulation functions, that will make a whole
bunch more of the libc dependencies drop out.

So the bottom line is yes, I think we can do somethings to help you;
but depending on which parts of the libext2fs functionality yaboot
actually needs, it may not be as bad as you think, especially if you
are willing to limit which libext2fs functions you call.

Regards,

						- Ted

> 
> Our current set looks like:
> ---
> int printf(const char *format, ...);
> int fprintf(FILE *stream, const char *format, ...);
> int fputs(const char *s, FILE *stream);
> int fflush(FILE *stream);
> char *getenv(const char *name);
> int gethostname(char *name, size_t len);
> int gettimeofday(struct timeval *tv, struct timezone *tz);
> int * __errno_location(void);
> unsigned int sleep(unsigned int seconds);
> int rand(void);
> void srand(unsigned int seed);
> long int random(void);
> void srandom(unsigned int seed);
> uid_t geteuid(void);
> uid_t getuid(void);
> pid_t getpid(void);
> int stat(const char *path, struct stat *buf);
> int stat64(const char *path, struct stat *buf);
> int fstat(int fd, struct stat *buf);
> int fstat64(int fd, struct stat *buf);
> int open(const char *pathname, int flags, mode_t mode);
> int open64(const char *pathname, int flags, mode_t mode);
> off_t lseek(int fd, off_t offset, int whence);
> off64_t lseek64(int fd, off64_t offset, int whence);
> ssize_t read(int fildes, void *buf, size_t nbyte);
> int close(int fd);
> void *calloc(size_t nmemb, size_t size);
> void perror(const char *s);
> void exit(int status);
> int ioctl(int d, int request, ...);
> size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
> long sysconf(int name);
> int getpagesize(void);
> void qsort(void *base, size_t nmemb, size_t size,
>            int(*compar)(const void *, const void *));
> ---
> 
> Plus a few typdefs and #defines.
> 
> It looks to me (with only a quick look) that a lot of these libc
> functions are used for nice error messages to the end user.
> 
> So my question is essentially twofold.
> 1) Is there a "good" way to ./configure e2fsprogs to build a really
> minimal libext2fs.a ?
> 2) If not can I get some pointers adding a --really-minimal autoconf
> argument to pretty much do what I need.
> 
> Of course getting e2fsprogs to support this minimal build is only the
> first step I then need to convince the distros to package it.
> 
> I did consider, cp'ing the code into yaboot but that seems pretty gross
> and would end up breaching the Fedora Packaging Guidelines.
> 
> Any advice gladly accepted.
> 
> Yours Tony

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 6780 bytes --]

On Fri, Jun 15, 2012 at 08:08:25PM -0400, Ted Ts'o wrote:

> So I think you need to break your list a bit more.  There are a number
> of functions on this list where, if yaboot doesn't actually call a
> particular function (for example, the progess bar functions, in
> lib/ext2fs/progress.c), the fact that they reference the stdio
> functions won't matter.  Something similar will be going on with
> test_io.c; if you don't use those functions, you won't need to provide
> stubs for any of the libc functions called by them.

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?

What have I misunderstood?

> There are some cases where we may need to create some configure
> options.  For example, there are a large number of the symbols that
> you have listed that got dragged in by the lib/ext2fs/mmp.c.  That's
> one where it may be simplest to add a set of #ifdef's that comment out
> multi-mount protection --- I assume you don't plan to support booting
> over fibre channel where you might need to care about having two
> systems trying to modify the same file system at the same time.  :-)

No we really don't need mmp :)  I can see creating --enable-mmp and
--disbale-mmp options for configure would be reasonable and not too
intrusive, and it gets rid of nearly 30% of the libc functions.

Something like this lightly tested patch:
---
diff --git a/MCONFIG.in b/MCONFIG.in
index fa2b03e..a0c828a 100644
--- a/MCONFIG.in
+++ b/MCONFIG.in
@@ -53,7 +53,7 @@ datadir = @datadir@
 CC = @CC@
 BUILD_CC = @BUILD_CC@
 CFLAGS = @CFLAGS@
-CPPFLAGS = @INCLUDES@
+CPPFLAGS = @INCLUDES@ $(ENABLE_MPP)
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 LDFLAGS = @LDFLAGS@
 ALL_LDFLAGS = $(LDFLAGS) @LDFLAG_DYNAMIC@
diff --git a/configure.in b/configure.in
index 7373e8e..f7562d3 100644
--- a/configure.in
+++ b/configure.in
@@ -746,6 +746,24 @@ AC_MSG_RESULT([Building uuidd by default])
 )
 AC_SUBST(UUIDD_CMT)
 dnl
+dnl handle --disable-mmp
+dnl
+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])
+	MMP_CMT=
+fi
+,
+AC_MSG_RESULT([Enabling mmp support by default])
+MMP_CMT=
+)
+AC_SUBST(MMP_CMT)
+dnl
 dnl
 dnl
 MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library
diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index 0d9ac21..021f3b0 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -14,9 +14,11 @@ 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
+@MMP_CMT@ENABLE_MMP = -DENABLE_MMP
 
 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 +68,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/ext2fs.h b/lib/ext2fs/ext2fs.h
index ff088bb..cfa8822 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 ENABLE_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,22 @@ 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);
+#else
+static errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_clear(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_init(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_start(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_update(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_stop(ext2_filsys fs)
+{ return (unsigned)0; }
+#endif /* ENABLE_MMP */
 
 /* read_bb.c */
 extern errcode_t ext2fs_read_bb_inode(ext2_filsys fs,
---

Feedback very welcome but if it looks okay I'll add my signed off and
resubmit with a reasonable commit message.
 
> Which brings up another point --- are you only planning on opening the
> file system read-only, or do you expect to modify the file system from
> yaboot?  If you don't need to modify the file system, and so you don't
> need to load the bitmap manipulation functions, that will make a whole
> bunch more of the libc dependencies drop out.

Well currently we open it read-write but I cannot see a problem with
switching to read-only.  Based on my very limited understanding of
e2fsprogs it seems that disabling the bitmap functions is good for yaboot
it would result in a library that has pretty limited value to anyone
else.

A really ugly local hack to disbale the bitmap functions (along with the
patch above) results in calloc() being the only missing symbol.  Adding
calloc is probably a good idea anyway :)
 
> So the bottom line is yes, I think we can do somethings to help you;
> but depending on which parts of the libext2fs functionality yaboot
> actually needs, it may not be as bad as you think, especially if you
> are willing to limit which libext2fs functions you call.

We're a very simple user.  We only support ext* on local disks so no
fibre channel or anything too fancy.

For the record we currently only call:
	ext2fs_open()
	ext2fs_namei_follow()
	ext2fs_follow_link()
	ext2fs_read_inode()
	ext2fs_close()
	ext2fs_block_iterate()
	ext2fs_bmap() # Actually we may not use this anymore 

I'm also happy to rewrite the yaboot code for ext* if someone with
more knowledge can make a provide pointers.

I'd like to keep the headaches involved with using yaboot "small", so if
I can do a little extra work now to ensure that users not need to really
care what options (dir_index etc) the file-system is created with I'm
all for that.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Ted Ts'o]

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.

> No we really don't need mmp :)  I can see creating --enable-mmp and
> --disbale-mmp options for configure would be reasonable and not too
> intrusive, and it gets rid of nearly 30% of the libc functions.

Yes, absolutely.  I'll look at your patch but adding --disable-mmp is
something I'm very comfortable adding to e2fsprogs.

> Feedback very welcome but if it looks okay I'll add my signed off and
> resubmit with a reasonable commit message.

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.

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.

> Well currently we open it read-write but I cannot see a problem with
> switching to read-only.  Based on my very limited understanding of
> e2fsprogs it seems that disabling the bitmap functions is good for yaboot
> it would result in a library that has pretty limited value to anyone
> else.

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.

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.

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.

> For the record we currently only call:
> 	ext2fs_open()
> 	ext2fs_namei_follow()
> 	ext2fs_follow_link()
> 	ext2fs_read_inode()
> 	ext2fs_close()
> 	ext2fs_block_iterate()
> 	ext2fs_bmap() # Actually we may not use this anymore 
> 
> I'm also happy to rewrite the yaboot code for ext* if someone with
> more knowledge can make a provide pointers.

OK, that's useful.  I'm a little busy at the moment, but in the next
couple of weeks as I have time I'll trace down what is getting dragged
in and see if I can minimize the number of library functions that are
pulled in via static library link, and where necessary add configure
options to disable stuff that you wouldn't need.

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.

Regards,

						- Ted

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 15641 bytes --]

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(-)

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
 }
 
 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
 	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 */
 
 /* 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) {
 		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


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 15704 bytes --]

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






[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: Minimal configuration for e2fsprogs

[Ted Ts'o]

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?

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 10941 bytes --]

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(-)

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 #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

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






[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Mon, Jun 18, 2012 at 01:12:57PM -0400, Ted Ts'o wrote:

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

I'm ready to tackle the --disable-libext2fs-stats portion but I need to
make sure I'm looking in the rigth area.

I was thinking that this was the very sinmple matter of making
BMAP_STATS conditional rather that have it always #defined.  Is that
what you were thinking?

Also what's the correct way to get BMAP_STATS_OPS defined?

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Theodore Ts'o]

On Tue, Jun 26, 2012 at 12:10:47PM +1000, Tony Breeds wrote:
> 
> I was thinking that this was the very sinmple matter of making
> BMAP_STATS conditional rather that have it always #defined.  Is that
> what you were thinking?
> 
> Also what's the correct way to get BMAP_STATS_OPS defined?

Yeah, I was thinking to simply rename BMAP_STATS and BMAP_STATS_OPS to
ENABLE_BMAP_STATS and ENABLE_BMAP_STATS_OPS, and then handle them via
an AC_ARG_ENABLE much like --enable-compression is handled (with the
first enabled by default, so --disable-bmap-stats to the configure
script would disable it, while the much more heavier weight from a
performance standpoint BMAP_STATS_OPS would be disabled by default, so
the user would have to specify --enable-bmap-stats-ops to the
configure script to enable it.)

Regards,

						- Ted

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Mon, Jun 25, 2012 at 10:33:31PM -0400, Theodore Ts'o wrote:
> On Tue, Jun 26, 2012 at 12:10:47PM +1000, Tony Breeds wrote:
> > 
> > I was thinking that this was the very sinmple matter of making
> > BMAP_STATS conditional rather that have it always #defined.  Is that
> > what you were thinking?
> > 
> > Also what's the correct way to get BMAP_STATS_OPS defined?
> 
> Yeah, I was thinking to simply rename BMAP_STATS and BMAP_STATS_OPS to
> ENABLE_BMAP_STATS and ENABLE_BMAP_STATS_OPS, and then handle them via
> an AC_ARG_ENABLE much like --enable-compression is handled (with the
> first enabled by default, so --disable-bmap-stats to the configure
> script would disable it, while the much more heavier weight from a
> performance standpoint BMAP_STATS_OPS would be disabled by default, so
> the user would have to specify --enable-bmap-stats-ops to the
> configure script to enable it.)

Thanks for the clarification.  Should be doable today.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 3006 bytes --]

On Fri, Jun 15, 2012 at 02:24:21PM +1000, Tony Breeds wrote:
> Hi All,
> 	I appologise if this is not the correct place to discuss this,
> or if it's been discussed before.  In either event please point me in
> the right direction and I'll move along.
> 
> I'm the maintainer for yaboot a bootloader for powerpc systems.  We link
> against libext2fs.a, but as we're a bootloader we do NOT link against
> libc as such we need to implement a number of "stub" functions to keep
> up with the newer features being added here.

Okay working with an e2fsprogs that has the various patches I've posted
to this list in the last few days I'm down to:
---
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(inline.o): In function `ext2fs_get_arrayzero':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/ext2fs.h:1521: undefined reference to `calloc'
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(blkmap64_rb.o): In function `rb_get_new_extent':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/blkmap64_rb.c:138: undefined reference to `perror'
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/blkmap64_rb.c:139: undefined reference to `exit'
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(dblist.o): In function `ext2fs_dblist_sort2':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/dblist.c:217: undefined reference to `qsort'
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/dblist.c:217: undefined reference to `qsort'
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(dblist.o): In function `ext2fs_dblist_sort':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/dblist.c:322: undefined reference to `qsort'
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/dblist.c:322: undefined reference to `qsort'
---
As I've said adding calloc to yaboot is quite reasonable so we can
ignore this one.

Calling perror/exit from this deep in a library doesn't seem right to me I
think a better option would be to change rb_get_new_extent()
to return an errcode_t and pass that up the call chain.  I'm happy to do
that.  If I read rb_insert_extent() correctly I can simply return if
rb_get_new_extent() failed, as nothing as been changed at this point
you've only traversed the rb tree.  The problem is that very few of the
callers of rb_insert_extent() actually check the return value :( so this
patch will be a little bigger than I'd like.

The qsort calls scare me a little.  I expect that bad things would
happen if the directory block wasn't sorted.  So just providing a
qsort() in yaboot that does nothing would be a bad thing.  I'm
kind of hoping you'll just say "as long as you're opening the
file-system read-only the directory block will be sorted so don't sweat
it"  Am I dreaming?

As always happy to do what I can if pointed in the right direction.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Theodore Ts'o]

On Wed, Jun 27, 2012 at 09:21:17PM +1000, Tony Breeds wrote:
> 
> Calling perror/exit from this deep in a library doesn't seem right to me I
> think a better option would be to change rb_get_new_extent()
> to return an errcode_t and pass that up the call chain.  I'm happy to do
> that.  If I read rb_insert_extent() correctly I can simply return if
> rb_get_new_extent() failed, as nothing as been changed at this point
> you've only traversed the rb tree.  The problem is that very few of the
> callers of rb_insert_extent() actually check the return value :( so this
> patch will be a little bigger than I'd like.

Well, rb_insert_extent() isn't returning an error; it's returning
whether or not it needed to insert an extent or not.  And the bigger
problem is there's no way to return an error all the way up the
callchain, since it ultimately gets called by functions like
ext2fs_mark_block_bitmap() which never contemplated that the function
might fail.

So there really is no way to return an error at this point, and if you
fail allocating memory, we're kind of doomed anyway.  We could replace
this with an abort(), but there's really not much else we can do here.

I suppose, more generally, we could add a new callback which gets
called on memory allocation failures; although in practice, the place
where we are most likely to run into trouble is e2fsck, and we already
have sufficient debugging code there thanks to e2fsck/sigcatcher.c.
So maybe just using an abort() is the best approach for now.

> The qsort calls scare me a little.  I expect that bad things would
> happen if the directory block wasn't sorted.  So just providing a
> qsort() in yaboot that does nothing would be a bad thing.  I'm
> kind of hoping you'll just say "as long as you're opening the
> file-system read-only the directory block will be sorted so don't sweat
> it"  Am I dreaming?

So I believe the only place where the dblist.o file is getting dragged
in is due to the call to ext2fs_free_dblist() in freefs.c.  Can you
confirm this?

If so, probably the way to solve this is the via the same trick we do
with fs->write_bitmaps() --- see how we only call fs->write_bitmaps()
if it is defined in ext2fs_close2(); this was done precisely to avoid
dragging in the write_bitmaps code if it's not needed for programs
which are opening the file system read/only.

Regards,

						- Ted

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

On Wed, Jun 27, 2012 at 08:54:43AM -0400, Theodore Ts'o wrote:
 
> Well, rb_insert_extent() isn't returning an error; it's returning
> whether or not it needed to insert an extent or not.  And the bigger
> problem is there's no way to return an error all the way up the
> callchain, since it ultimately gets called by functions like
> ext2fs_mark_block_bitmap() which never contemplated that the function
> might fail.

Okay.  I shoudl have read more carefully.
 
> So there really is no way to return an error at this point, and if you
> fail allocating memory, we're kind of doomed anyway.  We could replace
> this with an abort(), but there's really not much else we can do here.
> 
> I suppose, more generally, we could add a new callback which gets
> called on memory allocation failures; although in practice, the place
> where we are most likely to run into trouble is e2fsck, and we already
> have sufficient debugging code there thanks to e2fsck/sigcatcher.c.
> So maybe just using an abort() is the best approach for now.

Okay so I think you want somethign like:
---
diff --git a/e2fsck/sigcatcher.c b/e2fsck/sigcatcher.c
index 10b9328..bd56c3f 100644
--- a/e2fsck/sigcatcher.c
+++ b/e2fsck/sigcatcher.c
@@ -387,6 +387,7 @@ void sigcatcher_setup(void)
 	sigaction(SIGILL, &sa, 0);
 	sigaction(SIGBUS, &sa, 0);
 	sigaction(SIGSEGV, &sa, 0);
+	sigaction(SIGABRT, &sa, 0);
 }	
 
 
diff --git a/lib/ext2fs/blkmap64_rb.c b/lib/ext2fs/blkmap64_rb.c
index e6b7e04..74140ec 100644
--- a/lib/ext2fs/blkmap64_rb.c
+++ b/lib/ext2fs/blkmap64_rb.c
@@ -134,10 +134,8 @@ static void rb_get_new_extent(struct bmap_rb_extent **ext, __u64 start,
 
 	retval = ext2fs_get_mem(sizeof (struct bmap_rb_extent),
 				&new_ext);
-	if (retval) {
-		perror("ext2fs_get_mem");
-		exit(1);
-	}
+	if (retval)
+		abort();
 
 	new_ext->start = start;
 	new_ext->count = count;
---

Adding something that calls itself abort() wont be hard in yaboot.

> So I believe the only place where the dblist.o file is getting dragged
> in is due to the call to ext2fs_free_dblist() in freefs.c.  Can you
> confirm this?

What I did was remove dblist.o from my libext2fs.a and I now get:
/home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(gen_bitmap64.o): In function `ext2fs_alloc_generic_bmap':
/home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/gen_bitmap64.c:112: undefined reference to `ext2fs_get_num_dirs'

So it looks like dblist.o is being pulled in for ext2fs_get_num_dirs()
in that case a EXT2FS_BMAP64_AUTODIR bmap type has been asked for.

How would you feel about moving ext2fs_get_num_dirs from dblist.c to
blknum.c?

> If so, probably the way to solve this is the via the same trick we do
> with fs->write_bitmaps() --- see how we only call fs->write_bitmaps()
> if it is defined in ext2fs_close2(); this was done precisely to avoid
> dragging in the write_bitmaps code if it's not needed for programs
> which are opening the file system read/only.

I didn't really look at this because of what I discovered above.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Theodore Ts'o]

On Thu, Jun 28, 2012 at 12:43:56PM +1000, Tony Breeds wrote:
> 
> Okay so I think you want somethign like:

Yep!

> What I did was remove dblist.o from my libext2fs.a and I now get:
> /home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(gen_bitmap64.o): In function `ext2fs_alloc_generic_bmap':
> /home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/gen_bitmap64.c:112: undefined reference to `ext2fs_get_num_dirs'
> 
> So it looks like dblist.o is being pulled in for ext2fs_get_num_dirs()
> in that case a EXT2FS_BMAP64_AUTODIR bmap type has been asked for.
> 
> How would you feel about moving ext2fs_get_num_dirs from dblist.c to
> blknum.c?

Sure, no problem.

Take a look at this set of patches (see attached).  With this and the
set of ext2fs functions that I understand yaboot is using, I think it
solves the concern that you have.  This will be in the next branch of
the e2fsprogs.

Regards,

					- Ted

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

On Mon, Jul 30, 2012 at 05:45:32PM -0400, Theodore Ts'o wrote:
> On Thu, Jun 28, 2012 at 12:43:56PM +1000, Tony Breeds wrote:
> > 
> > Okay so I think you want somethign like:
> 
> Yep!
> 
> > What I did was remove dblist.o from my libext2fs.a and I now get:
> > /home/tony/src/e2fsprogs/../e2fsprogs-root/lib/libext2fs.a(gen_bitmap64.o): In function `ext2fs_alloc_generic_bmap':
> > /home/tony/src/e2fsprogs-build/lib/ext2fs/../../../e2fsprogs/lib/ext2fs/gen_bitmap64.c:112: undefined reference to `ext2fs_get_num_dirs'
> > 
> > So it looks like dblist.o is being pulled in for ext2fs_get_num_dirs()
> > in that case a EXT2FS_BMAP64_AUTODIR bmap type has been asked for.
> > 
> > How would you feel about moving ext2fs_get_num_dirs from dblist.c to
> > blknum.c?
> 
> Sure, no problem.
> 
> Take a look at this set of patches (see attached).  With this and the
> set of ext2fs functions that I understand yaboot is using, I think it
> solves the concern that you have.  This will be in the next branch of
> the e2fsprogs.

Thanks so much Ted for your help with my patches and the next set.

I tried next + your 7 patches and yaboot only fails with errors for
calloc and abort.  Both of these I'm very happy to fix in yaboot.

Looking forward to v1.42.6 :)

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Minimal configuration for e2fsprogs

[Theodore Ts'o]

On Tue, Jul 31, 2012 at 03:21:50PM +1000, Tony Breeds wrote:
> Thanks so much Ted for your help with my patches and the next set.
> 
> I tried next + your 7 patches and yaboot only fails with errors for
> calloc and abort.  Both of these I'm very happy to fix in yaboot.
> 
> Looking forward to v1.42.6 :)

Unfortunately, all of these changes put together were significant
enough that I didn't feel comfortable putting them in the 1.42.x
maintenance branch, especially since Debian is going through its
freeze process and I'm trying to get 1.42.5 into the release.  So I'm
being a lot more conservative about what sort of changes I'm allowing
into the maintenance branch at this point.

The 'master' and 'next' branches in git are targetting the 1.43
release of e2fsprogs.  If you need a tarball, and not just git tree
reference, I could cut a pre-release snapshot for testing purposes.  I
was planning on doing that soon for the metadata checksum and/or
inline data features in any case.

Regards,

						- Ted

Re: Minimal configuration for e2fsprogs

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Tue, Jul 31, 2012 at 03:57:26PM -0400, Theodore Ts'o wrote:

> Unfortunately, all of these changes put together were significant
> enough that I didn't feel comfortable putting them in the 1.42.x
> maintenance branch, especially since Debian is going through its
> freeze process and I'm trying to get 1.42.5 into the release.  So I'm
> being a lot more conservative about what sort of changes I'm allowing
> into the maintenance branch at this point.

Okay.
 
> The 'master' and 'next' branches in git are targetting the 1.43
> release of e2fsprogs.  If you need a tarball, and not just git tree
> reference, I could cut a pre-release snapshot for testing purposes.  I
> was planning on doing that soon for the metadata checksum and/or
> inline data features in any case.

I don't think I specifically need it, but if I see an RC or testing
snapshot I'll certainly use it :)

Again thanks for your help on this.

Yours Tony

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/7] e2fsck: add SIGABRT to list of signals processed by sigcatcher

[Theodore Ts'o]

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 e2fsck/sigcatcher.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/e2fsck/sigcatcher.c b/e2fsck/sigcatcher.c
index 10b9328..bd56c3f 100644
--- a/e2fsck/sigcatcher.c
+++ b/e2fsck/sigcatcher.c
@@ -387,6 +387,7 @@ void sigcatcher_setup(void)
 	sigaction(SIGILL, &sa, 0);
 	sigaction(SIGBUS, &sa, 0);
 	sigaction(SIGSEGV, &sa, 0);
+	sigaction(SIGABRT, &sa, 0);
 }	
 
 
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 2/7] libext2fs: use abort() instead of perror()/exit()

[Theodore Ts'o]

This simplifies the number of C library symbols needed by boot loader
systems such as yaboot.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/blkmap64_rb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/ext2fs/blkmap64_rb.c b/lib/ext2fs/blkmap64_rb.c
index e6b7e04..74140ec 100644
--- a/lib/ext2fs/blkmap64_rb.c
+++ b/lib/ext2fs/blkmap64_rb.c
@@ -134,10 +134,8 @@ static void rb_get_new_extent(struct bmap_rb_extent **ext, __u64 start,
 
 	retval = ext2fs_get_mem(sizeof (struct bmap_rb_extent),
 				&new_ext);
-	if (retval) {
-		perror("ext2fs_get_mem");
-		exit(1);
-	}
+	if (retval)
+		abort();
 
 	new_ext->start = start;
 	new_ext->count = count;
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 2/7] libext2fs: use abort() instead of perror()/exit()

[Andreas Dilger]

On 2012-07-30, at 14:47, Theodore Ts'o <tytso@mit.edu> wrote:

> This simplifies the number of C library symbols needed by boot loader
> systems such as yaboot.

This doesn't improve the debugability of the code at all. Instead of getting an error message (as cryptic as it was), now there is no error and the process will just die.

I'm guessing from the original coding that there is no error handling for this case? 

Cheers, Andreas

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> lib/ext2fs/blkmap64_rb.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/ext2fs/blkmap64_rb.c b/lib/ext2fs/blkmap64_rb.c
> index e6b7e04..74140ec 100644
> --- a/lib/ext2fs/blkmap64_rb.c
> +++ b/lib/ext2fs/blkmap64_rb.c
> @@ -134,10 +134,8 @@ static void rb_get_new_extent(struct bmap_rb_extent **ext, __u64 start,
> 
>    retval = ext2fs_get_mem(sizeof (struct bmap_rb_extent),
>                &new_ext);
> -    if (retval) {
> -        perror("ext2fs_get_mem");
> -        exit(1);
> -    }
> +    if (retval)
> +        abort();
> 
>    new_ext->start = start;
>    new_ext->count = count;
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> 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

Re: [PATCH 2/7] libext2fs: use abort() instead of perror()/exit()

[Theodore Ts'o]

On Tue, Jul 31, 2012 at 11:34:38AM -0700, Andreas Dilger wrote:
> On 2012-07-30, at 14:47, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> > This simplifies the number of C library symbols needed by boot loader
> > systems such as yaboot.
> 
> This doesn't improve the debugability of the code at all. Instead of
> getting an error message (as cryptic as it was), now there is no
> error and the process will just die.

Well, at least for e2fsck, which is the program I was most concerned
about, the debuggability will actually improve, since
e2fsck/sigcatcher.c will give you a very nice stack backtrace (at
least, if your libc has the backtrace function).

> I'm guessing from the original coding that there is no error
> handling for this case?

Yes, the problem is that the ext2fs_{mark,unmark}_{block,inode}_bitmap()
functions return void, and changing this would require massive changes
all up and down the stack.

Even if they had originally return an errcode_t, given that with the
simple bit array implementation, they could Never Fail(tm), it's
likely that most if not all of the code sites would not have checked
them, and even if they did, all they could really do at that point is
die.  And if they didn't, then it would be even harder to debug why
the bitmap function was became a no-op due to a memory allocation
failure.

Sigh; I've become convinced that the Go language's philosphy not
letting memory allocation fail (and just simply dying if you can't
allocate the memory you need) is the Right Thing 99.99% of the time.


						- Ted

[PATCH 3/7] libext2fs: use strcpy()/strcat() instead of sprintf() in bmap functions

[Theodore Ts'o]

This simplifies the number of C library symbols needed by boot loader
systems such as yaboot.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/gen_bitmap64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
index 44b733d..dd2773a 100644
--- a/lib/ext2fs/gen_bitmap64.c
+++ b/lib/ext2fs/gen_bitmap64.c
@@ -322,7 +322,8 @@ errcode_t ext2fs_copy_generic_bmap(ext2fs_generic_bitmap src,
 			ext2fs_free_mem(&new_bmap);
 			return retval;
 		}
-		sprintf(new_descr, "copy of %s", descr);
+		strcpy(new_descr, "copy of ");
+		strcat(new_descr, descr);
 		new_bmap->description = new_descr;
 	}
 
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 4/7] libext2fs: move ext2fs_get_num_dirs to its own file

[Theodore Ts'o]

This reduces the number of C library symbols needed by boot loader
systems such as yaboot.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/Makefile.in    |  9 +++++++++
 lib/ext2fs/dblist.c       | 28 --------------------------
 lib/ext2fs/ext2fs.h       |  5 +++--
 lib/ext2fs/get_num_dirs.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 30 deletions(-)
 create mode 100644 lib/ext2fs/get_num_dirs.c

diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index 0d9ac21..c6835fe 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -50,6 +50,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
 	freefs.o \
 	gen_bitmap.o \
 	gen_bitmap64.o \
+	get_num_dirs.o \
 	get_pathname.o \
 	getsize.o \
 	getsectsize.o \
@@ -122,6 +123,7 @@ SRCS= ext2_err.c \
 	$(srcdir)/freefs.c \
 	$(srcdir)/gen_bitmap.c \
 	$(srcdir)/gen_bitmap64.c \
+	$(srcdir)/get_num_dirs.c \
 	$(srcdir)/get_pathname.c \
 	$(srcdir)/getsize.c \
 	$(srcdir)/getsectsize.c \
@@ -686,6 +688,13 @@ gen_bitmap64.o: $(srcdir)/gen_bitmap64.c $(top_builddir)/lib/config.h \
  $(top_srcdir)/lib/et/com_err.h $(srcdir)/ext2_io.h \
  $(top_builddir)/lib/ext2fs/ext2_err.h $(srcdir)/ext2_ext_attr.h \
  $(srcdir)/bitops.h $(srcdir)/bmap64.h
+get_num_dirs.o: $(srcdir)/get_num_dirs.c $(top_builddir)/lib/config.h \
+ $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
+ $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fsP.h \
+ $(srcdir)/ext2fs.h $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h \
+ $(top_srcdir)/lib/et/com_err.h $(srcdir)/ext2_io.h \
+ $(top_builddir)/lib/ext2fs/ext2_err.h $(srcdir)/ext2_ext_attr.h \
+ $(srcdir)/bitops.h
 get_pathname.o: $(srcdir)/get_pathname.c $(top_builddir)/lib/config.h \
  $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
  $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
diff --git a/lib/ext2fs/dblist.c b/lib/ext2fs/dblist.c
index ca1446b..ceaae8f 100644
--- a/lib/ext2fs/dblist.c
+++ b/lib/ext2fs/dblist.c
@@ -25,34 +25,6 @@ static EXT2_QSORT_TYPE dir_block_cmp2(const void *a, const void *b);
 static EXT2_QSORT_TYPE (*sortfunc32)(const void *a, const void *b);
 
 /*
- * Returns the number of directories in the filesystem as reported by
- * the group descriptors.  Of course, the group descriptors could be
- * wrong!
- */
-errcode_t ext2fs_get_num_dirs(ext2_filsys fs, ext2_ino_t *ret_num_dirs)
-{
-	dgrp_t	i;
-	ext2_ino_t	num_dirs, max_dirs;
-
-	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
-
-	num_dirs = 0;
-	max_dirs = fs->super->s_inodes_per_group;
-	for (i = 0; i < fs->group_desc_count; i++) {
-		if (ext2fs_bg_used_dirs_count(fs, i) > max_dirs)
-			num_dirs += max_dirs / 8;
-		else
-			num_dirs += ext2fs_bg_used_dirs_count(fs, i);
-	}
-	if (num_dirs > fs->super->s_inodes_count)
-		num_dirs = fs->super->s_inodes_count;
-
-	*ret_num_dirs = num_dirs;
-
-	return 0;
-}
-
-/*
  * helper function for making a new directory block list (for
  * initialize and copy).
  */
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 6b9a577..62282b1 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -938,8 +938,6 @@ extern errcode_t ext2fs_set_gdt_csum(ext2_filsys fs);
 extern __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group);
 
 /* dblist.c */
-
-extern errcode_t ext2fs_get_num_dirs(ext2_filsys fs, ext2_ino_t *ret_num_dirs);
 extern errcode_t ext2fs_init_dblist(ext2_filsys fs, ext2_dblist *ret_dblist);
 extern errcode_t ext2fs_add_dir_block(ext2_dblist dblist, ext2_ino_t ino,
 				      blk_t blk, int blockcnt);
@@ -1189,6 +1187,9 @@ errcode_t ext2fs_set_generic_bmap_range(ext2fs_generic_bitmap bmap,
 errcode_t ext2fs_convert_subcluster_bitmap(ext2_filsys fs,
 					   ext2fs_block_bitmap *bitmap);
 
+/* get_num_dirs.c */
+extern errcode_t ext2fs_get_num_dirs(ext2_filsys fs, ext2_ino_t *ret_num_dirs);
+
 /* getsize.c */
 extern errcode_t ext2fs_get_device_size(const char *file, int blocksize,
 					blk_t *retblocks);
diff --git a/lib/ext2fs/get_num_dirs.c b/lib/ext2fs/get_num_dirs.c
new file mode 100644
index 0000000..f5644f8
--- /dev/null
+++ b/lib/ext2fs/get_num_dirs.c
@@ -0,0 +1,50 @@
+/*
+ * get_num_dirs.c -- calculate number of directories
+ *
+ * Copyright 1997 by Theodore Ts'o
+ *
+ * %Begin-Header%
+ * This file may be redistributed under the terms of the GNU Library
+ * General Public License, version 2.
+ * %End-Header%
+ */
+
+#include "config.h"
+#include <stdio.h>
+#if HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+#include <string.h>
+#include <time.h>
+
+#include "ext2_fs.h"
+#include "ext2fsP.h"
+
+/*
+ * Returns the number of directories in the filesystem as reported by
+ * the group descriptors.  Of course, the group descriptors could be
+ * wrong!
+ */
+errcode_t ext2fs_get_num_dirs(ext2_filsys fs, ext2_ino_t *ret_num_dirs)
+{
+	dgrp_t	i;
+	ext2_ino_t	num_dirs, max_dirs;
+
+	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
+
+	num_dirs = 0;
+	max_dirs = fs->super->s_inodes_per_group;
+	for (i = 0; i < fs->group_desc_count; i++) {
+		if (ext2fs_bg_used_dirs_count(fs, i) > max_dirs)
+			num_dirs += max_dirs / 8;
+		else
+			num_dirs += ext2fs_bg_used_dirs_count(fs, i);
+	}
+	if (num_dirs > fs->super->s_inodes_count)
+		num_dirs = fs->super->s_inodes_count;
+
+	*ret_num_dirs = num_dirs;
+
+	return 0;
+}
+
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 5/7] libext2fs: call numeric_progress functions through a operations struct

[Theodore Ts'o]

Instead of calling ext2fs_numeric_progress_*() directly from closefs.c
and alloc_tables.c, call it via a operations structure which is only
initialized by the one program (mke2fs) which needs it.

This reduces the number of C library symbols needed by boot loader
systems such as yaboot.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/alloc_tables.c | 11 +++++++----
 lib/ext2fs/closefs.c      | 11 +++++++----
 lib/ext2fs/ext2fs.h       |  3 +++
 lib/ext2fs/ext2fsP.h      | 17 +++++++++++++++++
 lib/ext2fs/progress.c     |  6 ++++++
 misc/mke2fs.c             |  1 +
 6 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/lib/ext2fs/alloc_tables.c b/lib/ext2fs/alloc_tables.c
index 9f3d4e0..5e6e556 100644
--- a/lib/ext2fs/alloc_tables.c
+++ b/lib/ext2fs/alloc_tables.c
@@ -230,16 +230,19 @@ errcode_t ext2fs_allocate_tables(ext2_filsys fs)
 	dgrp_t		i;
 	struct ext2fs_numeric_progress_struct progress;
 
-	ext2fs_numeric_progress_init(fs, &progress, NULL,
-				     fs->group_desc_count);
+	if (fs->progress_ops && fs->progress_ops->init)
+		(fs->progress_ops->init)(fs, &progress, NULL,
+					 fs->group_desc_count);
 
 	for (i = 0; i < fs->group_desc_count; i++) {
-		ext2fs_numeric_progress_update(fs, &progress, i);
+		if (fs->progress_ops && fs->progress_ops->update)
+			(fs->progress_ops->update)(fs, &progress, i);
 		retval = ext2fs_allocate_group_table(fs, i, fs->block_map);
 		if (retval)
 			return retval;
 	}
-	ext2fs_numeric_progress_close(fs, &progress, NULL);
+	if (fs->progress_ops && fs->progress_ops->close)
+		(fs->progress_ops->close)(fs, &progress, NULL);
 	return 0;
 }
 
diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
index 973c2a2..bb7f8b3 100644
--- a/lib/ext2fs/closefs.c
+++ b/lib/ext2fs/closefs.c
@@ -340,14 +340,16 @@ errcode_t ext2fs_flush2(ext2_filsys fs, int flags)
 	else
 		old_desc_blocks = fs->desc_blocks;
 
-	ext2fs_numeric_progress_init(fs, &progress, NULL,
-				     fs->group_desc_count);
+	if (fs->progress_ops && fs->progress_ops->init)
+		(fs->progress_ops->init)(fs, &progress, NULL,
+					 fs->group_desc_count);
 
 
 	for (i = 0; i < fs->group_desc_count; i++) {
 		blk64_t	super_blk, old_desc_blk, new_desc_blk;
 
-		ext2fs_numeric_progress_update(fs, &progress, i);
+		if (fs->progress_ops && fs->progress_ops->update)
+			(fs->progress_ops->update)(fs, &progress, i);
 		ext2fs_super_and_bgd_loc2(fs, i, &super_blk, &old_desc_blk,
 					 &new_desc_blk, 0);
 
@@ -376,7 +378,8 @@ errcode_t ext2fs_flush2(ext2_filsys fs, int flags)
 		}
 	}
 
-	ext2fs_numeric_progress_close(fs, &progress, NULL);
+	if (fs->progress_ops && fs->progress_ops->close)
+		(fs->progress_ops->close)(fs, &progress, NULL);
 
 	/*
 	 * If the write_bitmaps() function is present, call it to
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 62282b1..5746b4f 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -267,6 +267,9 @@ struct struct_ext2_filsys {
 	 * Time at which e2fsck last updated the MMP block.
 	 */
 	long mmp_last_written;
+
+	/* progress operation functions */
+	struct ext2fs_progress_ops *progress_ops;
 };
 
 #if EXT2_FLAT_INCLUDES
diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h
index 729d5c5..bdfb85e 100644
--- a/lib/ext2fs/ext2fsP.h
+++ b/lib/ext2fs/ext2fsP.h
@@ -95,6 +95,23 @@ struct ext2fs_numeric_progress_struct {
 	int		skip_progress;
 };
 
+/*
+ * progress callback functions
+ */
+struct ext2fs_progress_ops {
+	void (*init)(ext2_filsys fs,
+		     struct ext2fs_numeric_progress_struct * progress,
+		     const char *label, __u64 max);
+	void (*update)(ext2_filsys fs,
+		       struct ext2fs_numeric_progress_struct * progress,
+		       __u64 val);
+	void (*close)(ext2_filsys fs,
+		      struct ext2fs_numeric_progress_struct * progress,
+		      const char *message);
+};
+
+extern struct ext2fs_progress_ops ext2fs_numeric_progress_ops;
+
 extern void ext2fs_numeric_progress_init(ext2_filsys fs,
 					 struct ext2fs_numeric_progress_struct * progress,
 					 const char *label, __u64 max);
diff --git a/lib/ext2fs/progress.c b/lib/ext2fs/progress.c
index 37d1509..432816b 100644
--- a/lib/ext2fs/progress.c
+++ b/lib/ext2fs/progress.c
@@ -16,6 +16,12 @@
 
 static char spaces[80], backspaces[80];
 
+struct ext2fs_progress_ops ext2fs_numeric_progress_ops = {
+	.init		= ext2fs_numeric_progress_init,
+	.update		= ext2fs_numeric_progress_update,
+	.close		= ext2fs_numeric_progress_close,
+};
+
 static int int_log10(unsigned int arg)
 {
 	int	l;
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 7ec8cc2..01b2111 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2277,6 +2277,7 @@ int main (int argc, char *argv[])
 		com_err(device_name, retval, _("while setting up superblock"));
 		exit(1);
 	}
+	fs->progress_ops = &ext2fs_numeric_progress_ops;
 
 	/* Can't undo discard ... */
 	if (!noaction && discard && (io_ptr != undo_io_manager)) {
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 6/7] libext2fs: remove debugging printf from ext2fs_group_desc_csum

[Theodore Ts'o]

This reduces the number of C library symbols needed by boot loader
systems such as yaboot.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/csum.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index 9fa3f24..aefa762 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -40,7 +40,7 @@ __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group)
 	if (size < EXT2_MIN_DESC_SIZE)
 		size = EXT2_MIN_DESC_SIZE;
 	if (size > sizeof(struct ext4_group_desc)) {
-		printf("%s: illegal s_desc_size(%zd)\n", __func__, size);
+		/* This should never happen, but cap it for safety's sake */
 		size = sizeof(struct ext4_group_desc);
 	}
 
-- 
1.7.12.rc0.22.gcdd159b

[PATCH 7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

[Theodore Ts'o]

Since various parts of the library depend on the value of s_desc_size,
check to make sure it is the correct, expected value based on the file
system features.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/ext2_err.et.in |  3 +++
 lib/ext2fs/openfs.c       | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index ccf1894..5987185 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -443,4 +443,7 @@ ec	EXT2_ET_MMP_CHANGE_ABORT,
 ec	EXT2_ET_MMP_OPEN_DIRECT,
 	"MMP: open with O_DIRECT failed"
 
+ec	EXT2_ET_BAD_DESC_SIZE,
+	"Block group descriptor size incorrect"
+
 	end
diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index 482e4ab..fbe9acd 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -263,6 +263,21 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 		retval = EXT2_ET_CORRUPT_SUPERBLOCK;
 		goto cleanup;
 	}
+
+	/* Enforce the block group descriptor size */
+	if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
+		if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
+			retval = EXT2_ET_BAD_DESC_SIZE;
+			goto cleanup;
+		}
+	} else {
+		if (fs->super->s_desc_size &&
+		    fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) {
+			retval = EXT2_ET_BAD_DESC_SIZE;
+			goto cleanup;
+		}
+	}
+
 	fs->cluster_ratio_bits = fs->super->s_log_cluster_size -
 		fs->super->s_log_block_size;
 	if (EXT2_BLOCKS_PER_GROUP(fs->super) !=
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

[Andreas Dilger]

On 2012-07-30, at 14:47, Theodore Ts'o <tytso@mit.edu> wrote:
> Since various parts of the library depend on the value of s_desc_size,
> check to make sure it is the correct, expected value based on the file
> system features.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> lib/ext2fs/ext2_err.et.in |  3 +++
> lib/ext2fs/openfs.c       | 15 +++++++++++++++
> 2 files changed, 18 insertions(+)
> 
> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> index ccf1894..5987185 100644
> --- a/lib/ext2fs/ext2_err.et.in
> +++ b/lib/ext2fs/ext2_err.et.in
> @@ -443,4 +443,7 @@ ec    EXT2_ET_MMP_CHANGE_ABORT,
> ec    EXT2_ET_MMP_OPEN_DIRECT,
>    "MMP: open with O_DIRECT failed"
> 
> +ec    EXT2_ET_BAD_DESC_SIZE,
> +    "Block group descriptor size incorrect"
> +
>    end
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index 482e4ab..fbe9acd 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -263,6 +263,21 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
>        retval = EXT2_ET_CORRUPT_SUPERBLOCK;
>        goto cleanup;
>    }
> +
> +    /* Enforce the block group descriptor size */
> +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {

It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 

Cheers, Andreas

> +            retval = EXT2_ET_BAD_DESC_SIZE;
> +            goto cleanup;
> +        }
> +    } else {
> +        if (fs->super->s_desc_size &&
> +            fs->super->s_desc_size != EXT2_MIN_DESC_SIZE) {
> +            retval = EXT2_ET_BAD_DESC_SIZE;
> +            goto cleanup;
> +        }
> +    }
> +
>    fs->cluster_ratio_bits = fs->super->s_log_cluster_size -
>        fs->super->s_log_block_size;
>    if (EXT2_BLOCKS_PER_GROUP(fs->super) !=
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> 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

Re: [PATCH 7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

[Theodore Ts'o]

On Tue, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote:
> > +    /* Enforce the block group descriptor size */
> > +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> > +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
> 
> It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 

I'm not at all convinced that the ext2fs library will do the right
thing if the block group size is larger than what we expect.  At the
very least we need to make sure it is a power of two, but even so I'd
want to audit the code and do some experiments before I would hang my
hat on this actually working correctly...

					- Ted

Re: [PATCH 7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

[Andreas Dilger]

On 2012-07-31, at 13:09, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jul 31, 2012 at 11:38:47AM -0700, Andreas Dilger wrote:
>>> +    /* Enforce the block group descriptor size */
>>> +    if (fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
>>> +        if (fs->super->s_desc_size != EXT2_MIN_DESC_SIZE_64BIT) {
>> 
>> It doesn't necessarily make sense to limit this to the minimum size, only that it is at least the minimum size. 
> 
> I'm not at all convinced that the ext2fs library will do the right
> thing if the block group size is larger than what we expect.  At the
> very least we need to make sure it is a power of two, but even so I'd
> want to audit the code and do some experiments before I would hang my
> hat on this actually working correctly...

I'm pretty sure that we've always checked that it is a power-of-two value. The problem with this change is that it makes it much harder to increase the size again in the future .

It would definitely need another feature flag, but it isn't clear without more investigation (that i can't do on my phone) if a COMPAT flag would be enough (which we would likely have anyway if we needed a larger descriptor) or if we need a more restrictive feature flag. 

Cheers, Andreas

Re: [PATCH 7/7] libext2fs: enforce the block group descriptor size in ext2fs_open()

[Theodore Ts'o]

On Wed, Aug 01, 2012 at 01:45:32PM -0700, Andreas Dilger wrote:
> 
> I'm pretty sure that we've always checked that it is a power-of-two
> value. The problem with this change is that it makes it much harder
> to increase the size again in the future .

Unfortunately, no, there aren't any such checks anywhere in either the
kernel or e2fsck.  In fact, e2fsck wasn't checking s_desc_size at all
(although it was depending on it), which is what caused me to get very
worried...

> It would definitely need another feature flag, but it isn't clear
> without more investigation (that i can't do on my phone) if a COMPAT
> flag would be enough (which we would likely have anyway if we needed
> a larger descriptor) or if we need a more restrictive feature flag.

It seems very likely that any new feature that would require us to
expand the block group descriptor would require other changes to the
file system's structures that would require a more restrictive
feature flag....

						- Ted