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

login
register
mail settings
Submitter Theodore Ts'o
Date July 30, 2012, 9:47 p.m.
Message ID <1343684862-13181-2-git-send-email-tytso@mit.edu>
Download mbox | patch
Permalink /patch/174105/
State Accepted
Headers show

Comments

Theodore Ts'o - July 30, 2012, 9:47 p.m.
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(-)
Andreas Dilger - July 31, 2012, 6:34 p.m.
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
--
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
Theodore Ts'o - July 31, 2012, 8:04 p.m.
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
--
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

Patch

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;