diff mbox

Minimal configuration for e2fsprogs

Message ID 20120628024356.GB17989@thor.bakeyournoodle.com
State Superseded, archived
Headers show

Commit Message

Tony Breeds June 28, 2012, 2:43 a.m. UTC 
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:
---
---

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

Comments

Theodore Ts'o July 30, 2012, 9:45 p.m. UTC | #1 
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
--
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
Tony Breeds July 31, 2012, 5:21 a.m. UTC | #2 
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
Theodore Ts'o July 31, 2012, 7:57 p.m. UTC | #3 
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
--
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
Tony Breeds Aug. 1, 2012, 5:42 a.m. UTC | #4 
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
diff mbox

Patch

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;