[PATCH 00/15] e2fsprogs cleanups

[PATCH 00/15] e2fsprogs cleanups

[Namhyung Kim]

Hi,

This is a series of small patches that I found during reading the
mke2fs code. Most of them are fix of (potential) memory leaks and
addition of more error checks. Plus a couple of dubious code fix
and trivial cleanups. The man page of mke2fs also be updated.

Any comments/suggestions are welcomed.
Thanks.

---

Namhyung Kim (15):
  libext2fs: fix potential build failure with OMIT_COM_ERR
  libext2fs: fix dubious code in ext2fs_numeric_progress_init()
  mke2fs: simplify inode table block counting
  libext2fs: remove unnecessary casts to ext2fs_generic_bitmap
  libext2fs: fix dubious code in ext2fs_unmark_generic_bitmap()
  libext2fs: invalid EXT4_FEATURE_RO_COMPAT_HUGE_FILE checks
  libext2fs: fix error path in ext2fs_update_bb_inode()
  libext2fs: fix memory leak on error path
  mke2fs: check return value of e2p_os2string()
  mke2fs.8.in: add missing "big" and "huge" usage-type description
  mke2fs: fix determination of size_type
  mke2fs: add some error checks into PRS()
  mke2fs: fix potential memory leak in mke2fs_setup_tdb()
  libext2fs: fix possible memory leak in write_journal_inode()
  mke2fs.8.in: add ENVIRONMENT section

 lib/ext2fs/bb_inode.c     |    6 ++-
 lib/ext2fs/blknum.c       |    4 +-
 lib/ext2fs/gen_bitmap.c   |   14 ++++----
 lib/ext2fs/gen_bitmap64.c |   69 +++++++++++++++++----------------------------
 lib/ext2fs/inode_io.c     |    4 ++-
 lib/ext2fs/mkjournal.c    |    1 +
 lib/ext2fs/progress.c     |    2 +-
 misc/mke2fs.8.in          |   34 +++++++++++++++++++++-
 misc/mke2fs.c             |   52 ++++++++++++++++++++++++----------
 9 files changed, 114 insertions(+), 72 deletions(-)

[PATCH 01/15 RESEND] libext2fs: fix potential build failure with OMIT_COM_ERR

[Namhyung Kim]

This fixes following build failure when OMIT_COM_ERR is defined:

 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_clear_generic_bitmap’:
 lib/ext2fs/gen_bitmap.c:437: error: invalid storage class for function ‘ext2fs_test_clear_generic_bitmap_range’
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_get_generic_bitmap_end’:
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_get_generic_bitmap_start’:
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_unmark_generic_bitmap’:
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_mark_generic_bitmap’:
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
 lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_test_generic_bitmap’:
 lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
make[2]: *** [gen_bitmap.o] Error 1
make[2]: Leaving directory e2fsprogs/lib/ext2fs'
make[1]: *** [all-libs-recursive] Error 1
make[1]: Leaving directory e2fsprogs'
make: *** [all] Error 2

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/gen_bitmap.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/ext2fs/gen_bitmap.c b/lib/ext2fs/gen_bitmap.c
index 99bf28d..eded435 100644
--- a/lib/ext2fs/gen_bitmap.c
+++ b/lib/ext2fs/gen_bitmap.c
@@ -178,9 +178,9 @@ int ext2fs_test_generic_bitmap(ext2fs_generic_bitmap bitmap,
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"test_bitmap(%lu)", (unsigned long) bitno);
+#endif
 		return 0;
 	}
-#endif
 
 	if ((bitno < bitmap->start) || (bitno > bitmap->end)) {
 		ext2fs_warn_bitmap2(bitmap, EXT2FS_TEST_ERROR, bitno);
@@ -200,9 +200,9 @@ int ext2fs_mark_generic_bitmap(ext2fs_generic_bitmap bitmap,
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"mark_bitmap(%lu)", (unsigned long) bitno);
+#endif
 		return 0;
 	}
-#endif
 
 	if ((bitno < bitmap->start) || (bitno > bitmap->end)) {
 		ext2fs_warn_bitmap2(bitmap, EXT2FS_MARK_ERROR, bitno);
@@ -222,9 +222,9 @@ int ext2fs_unmark_generic_bitmap(ext2fs_generic_bitmap bitmap,
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"mark_bitmap(%lu)", (unsigned long) bitno);
+#endif
 		return 0;
 	}
-#endif
 
 	if ((bitno < bitmap->start) || (bitno > bitmap->end)) {
 		ext2fs_warn_bitmap2(bitmap, EXT2FS_UNMARK_ERROR, bitno);
@@ -243,9 +243,9 @@ __u32 ext2fs_get_generic_bitmap_start(ext2fs_generic_bitmap bitmap)
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"get_bitmap_start");
+#endif
 		return 0;
 	}
-#endif
 
 	return bitmap->start;
 }
@@ -260,9 +260,9 @@ __u32 ext2fs_get_generic_bitmap_end(ext2fs_generic_bitmap bitmap)
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"get_bitmap_end");
+#endif
 		return 0;
 	}
-#endif
 	return bitmap->end;
 }
 
@@ -277,9 +277,9 @@ void ext2fs_clear_generic_bitmap(ext2fs_generic_bitmap bitmap)
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
 			"clear_generic_bitmap");
+#endif
 		return;
 	}
-#endif
 
 	memset(bitmap->bitmap, 0,
 	       (size_t) (((bitmap->real_end - bitmap->start) / 8) + 1));
-- 
1.7.0.4

--
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 02/15 RESEND] libext2fs: fix dubious code in ext2fs_numeric_progress_init()

[Namhyung Kim]

Looks like a copy & paste problem to me.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/progress.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/ext2fs/progress.c b/lib/ext2fs/progress.c
index 5ad2a45..ec4f553 100644
--- a/lib/ext2fs/progress.c
+++ b/lib/ext2fs/progress.c
@@ -40,7 +40,7 @@ void ext2fs_numeric_progress_init(ext2_filsys fs,
 	memset(spaces, ' ', sizeof(spaces)-1);
 	spaces[sizeof(spaces)-1] = 0;
 	memset(backspaces, '\b', sizeof(backspaces)-1);
-	spaces[sizeof(backspaces)-1] = 0;
+	backspaces[sizeof(backspaces)-1] = 0;
 	progress->skip_progress = 0;
 	if (getenv("E2FSPROGS_SKIP_PROGRESS"))
 		progress->skip_progress++;
-- 
1.7.0.4

[PATCH 03/15] mke2fs: simplify inode table block counting

[Namhyung Kim]

Simplify counting of inode table blocks in a block group using
local variable 'ipb'. Otherwise the variable could be removed
because there was no user of it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0980045..9fb5d5f 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -320,11 +320,8 @@ static void write_inode_tables(ext2_filsys fs, int lazy_flag, int itable_zeroed)
 
 		if (lazy_flag) {
 			ipb = fs->blocksize / EXT2_INODE_SIZE(fs->super);
-			num = ((((fs->super->s_inodes_per_group -
-				  ext2fs_bg_itable_unused(fs, i)) *
-				 EXT2_INODE_SIZE(fs->super)) +
-				EXT2_BLOCK_SIZE(fs->super) - 1) /
-			       EXT2_BLOCK_SIZE(fs->super));
+			num = (fs->super->s_inodes_per_group -
+			       ext2fs_bg_itable_unused(fs, i) + ipb - 1) / ipb;
 		}
 		if (!lazy_flag || itable_zeroed) {
 			/* The kernel doesn't need to zero the itable blocks */
-- 
1.7.0.4

[PATCH 04/15] libext2fs: remove unnecessary casts to ext2fs_generic_bitmap

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/gen_bitmap64.c |   69 +++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 43 deletions(-)

diff --git a/lib/ext2fs/gen_bitmap64.c b/lib/ext2fs/gen_bitmap64.c
index 9f23b92..eb87b6d 100644
--- a/lib/ext2fs/gen_bitmap64.c
+++ b/lib/ext2fs/gen_bitmap64.c
@@ -144,7 +144,7 @@ void ext2fs_free_generic_bmap(ext2fs_generic_bitmap bmap)
 		return;
 
 	if (EXT2FS_IS_32_BITMAP(bmap)) {
-		ext2fs_free_generic_bitmap((ext2fs_generic_bitmap) bmap);
+		ext2fs_free_generic_bitmap(bmap);
 		return;
 	}
 
@@ -171,8 +171,7 @@ errcode_t ext2fs_copy_generic_bmap(ext2fs_generic_bitmap src,
 		return EINVAL;
 
 	if (EXT2FS_IS_32_BITMAP(src))
-		return ext2fs_copy_generic_bitmap((ext2fs_generic_bitmap) src,
-						  (ext2fs_generic_bitmap *) dest);
+		return ext2fs_copy_generic_bitmap(src, dest);
 
 	if (!EXT2FS_IS_64_BITMAP(src))
 		return EINVAL;
@@ -222,11 +221,9 @@ errcode_t ext2fs_resize_generic_bmap(ext2fs_generic_bitmap bmap,
 	if (!bmap)
 		return EINVAL;
 
-	if (EXT2FS_IS_32_BITMAP(bmap)) {
-		return ext2fs_resize_generic_bitmap(bmap->magic,
-						    new_end, new_real_end,
-						    (ext2fs_generic_bitmap) bmap);
-	}
+	if (EXT2FS_IS_32_BITMAP(bmap))
+		return ext2fs_resize_generic_bitmap(bmap->magic, new_end,
+						    new_real_end, bmap);
 
 	if (!EXT2FS_IS_64_BITMAP(bmap))
 		return EINVAL;
@@ -245,7 +242,7 @@ errcode_t ext2fs_fudge_generic_bmap_end(ext2fs_generic_bitmap bitmap,
 		ext2_ino_t tmp_oend;
 		int retval;
 
-		retval = ext2fs_fudge_generic_bitmap_end((ext2fs_generic_bitmap) bitmap,
+		retval = ext2fs_fudge_generic_bitmap_end(bitmap,
 							 bitmap->magic, neq,
 							 end, &tmp_oend);
 		if (oend)
@@ -269,11 +266,8 @@ __u64 ext2fs_get_generic_bmap_start(ext2fs_generic_bitmap bitmap)
 	if (!bitmap)
 		return EINVAL;
 
-	if (EXT2FS_IS_32_BITMAP(bitmap)) {
-		return ext2fs_get_generic_bitmap_start((ext2fs_generic_bitmap)
-						       bitmap);
-
-	}
+	if (EXT2FS_IS_32_BITMAP(bitmap))
+		return ext2fs_get_generic_bitmap_start(bitmap);
 
 	if (!EXT2FS_IS_64_BITMAP(bitmap))
 		return EINVAL;
@@ -286,11 +280,8 @@ __u64 ext2fs_get_generic_bmap_end(ext2fs_generic_bitmap bitmap)
 	if (!bitmap)
 		return EINVAL;
 
-	if (EXT2FS_IS_32_BITMAP(bitmap)) {
-		return ext2fs_get_generic_bitmap_end((ext2fs_generic_bitmap)
-						       bitmap);
-
-	}
+	if (EXT2FS_IS_32_BITMAP(bitmap))
+		return ext2fs_get_generic_bitmap_end(bitmap);
 
 	if (!EXT2FS_IS_64_BITMAP(bitmap))
 		return EINVAL;
@@ -300,10 +291,8 @@ __u64 ext2fs_get_generic_bmap_end(ext2fs_generic_bitmap bitmap)
 
 void ext2fs_clear_generic_bmap(ext2fs_generic_bitmap bitmap)
 {
-	if (EXT2FS_IS_32_BITMAP(bitmap)) {
-		ext2fs_clear_generic_bitmap((ext2fs_generic_bitmap) bitmap);
-		return;
-	}
+	if (EXT2FS_IS_32_BITMAP(bitmap))
+		ext2fs_clear_generic_bitmap(bitmap);
 
 	bitmap->bitmap_ops->clear_bmap (bitmap);
 }
@@ -316,12 +305,11 @@ int ext2fs_mark_generic_bmap(ext2fs_generic_bitmap bitmap,
 
 	if (EXT2FS_IS_32_BITMAP(bitmap)) {
 		if (arg & ~0xffffffffULL) {
-			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bitmap,
+			ext2fs_warn_bitmap2(bitmap,
 					    EXT2FS_MARK_ERROR, 0xffffffff);
 			return 0;
 		}
-		return ext2fs_mark_generic_bitmap((ext2fs_generic_bitmap)
-						  bitmap, arg);
+		return ext2fs_mark_generic_bitmap(bitmap, arg);
 	}
 
 	if (!EXT2FS_IS_64_BITMAP(bitmap))
@@ -343,12 +331,11 @@ int ext2fs_unmark_generic_bmap(ext2fs_generic_bitmap bitmap,
 
 	if (EXT2FS_IS_32_BITMAP(bitmap)) {
 		if (arg & ~0xffffffffULL) {
-			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bitmap,
+			ext2fs_warn_bitmap2(bitmap,
 					    EXT2FS_UNMARK_ERROR, 0xffffffff);
 			return 0;
 		}
-		return ext2fs_unmark_generic_bitmap((ext2fs_generic_bitmap)
-						  bitmap, arg);
+		return ext2fs_unmark_generic_bitmap(bitmap, arg);
 	}
 
 	if (!EXT2FS_IS_64_BITMAP(bitmap))
@@ -370,12 +357,11 @@ int ext2fs_test_generic_bmap(ext2fs_generic_bitmap bitmap,
 
 	if (EXT2FS_IS_32_BITMAP(bitmap)) {
 		if (arg & ~0xffffffffULL) {
-			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bitmap,
+			ext2fs_warn_bitmap2(bitmap,
 					    EXT2FS_TEST_ERROR, 0xffffffff);
 			return 0;
 		}
-		return ext2fs_test_generic_bitmap((ext2fs_generic_bitmap)
-						  bitmap, arg);
+		return ext2fs_test_generic_bitmap(bitmap, arg);
 	}
 
 	if (!EXT2FS_IS_64_BITMAP(bitmap))
@@ -398,13 +384,12 @@ errcode_t ext2fs_set_generic_bmap_range(ext2fs_generic_bitmap bmap,
 
 	if (EXT2FS_IS_32_BITMAP(bmap)) {
 		if ((start+num) & ~0xffffffffULL) {
-			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bmap,
+			ext2fs_warn_bitmap2(bmap,
 					    EXT2FS_UNMARK_ERROR, 0xffffffff);
 			return EINVAL;
 		}
-		return ext2fs_set_generic_bitmap_range((ext2fs_generic_bitmap) bmap,
-						       bmap->magic, start, num,
-						       in);
+		return ext2fs_set_generic_bitmap_range(bmap, bmap->magic,
+						       start, num, in);
 	}
 
 	if (!EXT2FS_IS_64_BITMAP(bmap))
@@ -422,13 +407,12 @@ errcode_t ext2fs_get_generic_bmap_range(ext2fs_generic_bitmap bmap,
 
 	if (EXT2FS_IS_32_BITMAP(bmap)) {
 		if ((start+num) & ~0xffffffffULL) {
-			ext2fs_warn_bitmap2((ext2fs_generic_bitmap) bmap,
+			ext2fs_warn_bitmap2(bmap,
 					    EXT2FS_UNMARK_ERROR, 0xffffffff);
 			return EINVAL;
 		}
-		return ext2fs_get_generic_bitmap_range((ext2fs_generic_bitmap) bmap,
-						       bmap->magic, start, num,
-						       out);
+		return ext2fs_get_generic_bitmap_range(bmap, bmap->magic,
+						       start, num, out);
 	}
 
 	if (!EXT2FS_IS_64_BITMAP(bmap))
@@ -451,8 +435,7 @@ errcode_t ext2fs_compare_generic_bmap(errcode_t neq,
 	/* Now we know both bitmaps have the same magic */
 	if (EXT2FS_IS_32_BITMAP(bm1))
 		return ext2fs_compare_generic_bitmap(bm1->magic, neq,
-					     (ext2fs_generic_bitmap) bm1,
-					     (ext2fs_generic_bitmap) bm2);
+						     bm1, bm2);
 
 	if (!EXT2FS_IS_64_BITMAP(bm1))
 		return EINVAL;
@@ -474,7 +457,7 @@ void ext2fs_set_generic_bmap_padding(ext2fs_generic_bitmap bmap)
 	__u64	start, num;
 
 	if (EXT2FS_IS_32_BITMAP(bmap)) {
-		ext2fs_set_generic_bitmap_padding((ext2fs_generic_bitmap) bmap);
+		ext2fs_set_generic_bitmap_padding(bmap);
 		return;
 	}
 
-- 
1.7.0.4

[PATCH 05/15] libext2fs: fix dubious code in ext2fs_unmark_generic_bitmap()

[Namhyung Kim]

Looks like a copy & paste problem to me.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/gen_bitmap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/ext2fs/gen_bitmap.c b/lib/ext2fs/gen_bitmap.c
index eded435..3650013 100644
--- a/lib/ext2fs/gen_bitmap.c
+++ b/lib/ext2fs/gen_bitmap.c
@@ -217,7 +217,7 @@ int ext2fs_unmark_generic_bitmap(ext2fs_generic_bitmap bitmap,
 	if (!EXT2FS_IS_32_BITMAP(bitmap)) {
 		if (EXT2FS_IS_64_BITMAP(bitmap)) {
 			ext2fs_warn_bitmap32(bitmap, __func__);
-			return ext2fs_mark_generic_bmap(bitmap, bitno);
+			return ext2fs_unmark_generic_bmap(bitmap, bitno);
 		}
 #ifndef OMIT_COM_ERR
 		com_err(0, EXT2_ET_MAGIC_GENERIC_BITMAP,
-- 
1.7.0.4

[PATCH 06/15] libext2fs: invalid EXT4_FEATURE_RO_COMPAT_HUGE_FILE checks

[Namhyung Kim]

It should be checked with fs->super->s_feature_ro_compat instead of
->s_feature_incompat.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/blknum.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/blknum.c b/lib/ext2fs/blknum.c
index a48b696..b3e6dca 100644
--- a/lib/ext2fs/blknum.c
+++ b/lib/ext2fs/blknum.c
@@ -49,7 +49,7 @@ blk64_t ext2fs_inode_data_blocks2(ext2_filsys fs,
 					struct ext2_inode *inode)
 {
 	return (inode->i_blocks |
-		((fs->super->s_feature_incompat &
+		((fs->super->s_feature_ro_compat &
 		  EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ?
 		 (__u64) inode->osd2.linux2.l_i_blocks_hi << 32 : 0)) -
 		(inode->i_file_acl ? fs->blocksize >> 9 : 0);
@@ -62,7 +62,7 @@ blk64_t ext2fs_inode_i_blocks(ext2_filsys fs,
 					struct ext2_inode *inode)
 {
 	return (inode->i_blocks |
-		((fs->super->s_feature_incompat & 
+		((fs->super->s_feature_ro_compat &
 		  EXT4_FEATURE_RO_COMPAT_HUGE_FILE) ?
 		 (__u64)inode->osd2.linux2.l_i_blocks_hi << 32 : 0));
 }
-- 
1.7.0.4

[PATCH 07/15] libext2fs: fix error path in ext2fs_update_bb_inode()

[Namhyung Kim]

If ext2fs_get_mem() on rec.block_buf fails we should not call
ext2fs_free_mem() on it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/bb_inode.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/ext2fs/bb_inode.c b/lib/ext2fs/bb_inode.c
index 0b79b16..9576be2 100644
--- a/lib/ext2fs/bb_inode.c
+++ b/lib/ext2fs/bb_inode.c
@@ -74,8 +74,10 @@ errcode_t ext2fs_update_bb_inode(ext2_filsys fs, ext2_badblocks_list bb_list)
 		return retval;
 	memset(rec.ind_blocks, 0, rec.max_ind_blocks * sizeof(blk_t));
 	retval = ext2fs_get_mem(fs->blocksize, &rec.block_buf);
-	if (retval)
-		goto cleanup;
+	if (retval) {
+		ext2fs_free_mem(&rec.ind_blocks);
+		return retval;
+	}
 	memset(rec.block_buf, 0, fs->blocksize);
 	rec.err = 0;
 
-- 
1.7.0.4

[PATCH 08/15] libext2fs: fix memory leak on error path

[Namhyung Kim]

io->name should be freed if ext2fs_file_open2() fails.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/inode_io.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/ext2fs/inode_io.c b/lib/ext2fs/inode_io.c
index 4faaa48..bc934d3 100644
--- a/lib/ext2fs/inode_io.c
+++ b/lib/ext2fs/inode_io.c
@@ -157,11 +157,13 @@ static errcode_t inode_open(const char *name, int flags, io_channel *channel)
 				   &data->inode : 0, open_flags,
 				   &data->file);
 	if (retval)
-		goto cleanup;
+		goto cleanup_name;
 
 	*channel = io;
 	return 0;
 
+cleanup_name:
+	ext2fs_free_mem(&io->name);
 cleanup:
 	if (data) {
 		ext2fs_free_mem(&data);
-- 
1.7.0.4

[PATCH 09/15] mke2fs: check return value of e2p_os2string()

[Namhyung Kim]

e2p_os2string() calls malloc() so that it can return NULL.
Check it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 9fb5d5f..90cc206 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -554,6 +554,10 @@ static void show_stats(ext2_filsys fs)
 	printf(_("Filesystem label=%s\n"), buf);
 	fputs(_("OS type: "), stdout);
         os = e2p_os2string(fs->super->s_creator_os);
+	if (!os) {
+		fprintf(stderr, _("Couldn't allocate memory to show OS name!\n"));
+		exit(1);
+	}
 	fputs(os, stdout);
 	free(os);
 	printf("\n");
-- 
1.7.0.4

[PATCH 10/15] mke2fs.8.in: add missing "big" and "huge" usage-type description

[Namhyung Kim]

The commit 493024ea1d74e4cb48aac3a24111f5c8da343e9f ("mke2fs: Fix up the
fs type and feature selection for 64-bit blocks") added 'big' and 'huge'
usage-type but was missing description in man page. Add it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.8.in |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index b46e7e2..548ef66 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -616,8 +616,17 @@ will use the filesystem type
 If the filesystem size is greater than 3 but less than or equal to
 512 megabytes,
 .BR mke2fs (8)
-will use the filesystem
+will use the filesystem type
 .IR small .
+If the filesystem size is greater than or equal to 4 terabytes but less than
+16 terabytes,
+.BR mke2fs (8)
+will use the filesystem type
+.IR big .
+If the filesystem size is greater than or equal to 16 terabytes,
+.BR mke2fs (8)
+will use the filesystem type
+.IR huge .
 Otherwise,
 .BR mke2fs (8)
 will use the default filesystem type
-- 
1.7.0.4

[PATCH 11/15] mke2fs: fix determination of size_type

[Namhyung Kim]

In original code, 'huge' type could not be selected because it
always be caught for 'big' type. Change the ordering.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 90cc206..b88decf 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -947,10 +947,10 @@ static char **parse_fs_type(const char *fs_type,
 		size_type = "floppy";
 	else if (fs_blocks_count < 512 * meg)
 		size_type = "small";
-	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
-		size_type = "big";
 	else if (fs_blocks_count >= 16 * 1024 * 1024 * meg)
 		size_type = "huge";
+	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
+		size_type = "big";
 	else
 		size_type = "default";
 
-- 
1.7.0.4

[PATCH 12/15] mke2fs: add some error checks into PRS()

[Namhyung Kim]

Check return value of some functions and exit if unhandled error occurred.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index b88decf..6e2092d 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1108,6 +1108,10 @@ static void PRS(int argc, char *argv[])
 	if (oldpath)
 		pathlen += strlen(oldpath);
 	newpath = malloc(pathlen);
+	if (!newpath) {
+		fprintf(stderr, _("Couldn't allocate memory for new PATH.\n"));
+		exit(1);
+	}
 	strcpy(newpath, PATH_SET);
 
 	/* Update our PATH to include /sbin  */
@@ -1138,14 +1142,28 @@ static void PRS(int argc, char *argv[])
 	profile_set_syntax_err_cb(syntax_err_report);
 	retval = profile_init(config_fn, &profile);
 	if (retval == ENOENT) {
-		profile_init(default_files, &profile);
-		profile_set_default(profile, mke2fs_default_profile);
+		retval = profile_init(default_files, &profile);
+		if (retval)
+			goto profile_error;
+		retval = profile_set_default(profile, mke2fs_default_profile);
+		if (retval)
+			goto profile_error;
+	} else {
+profile_error:
+		fprintf(stderr, _("Couldn't init profile successfully"
+				  " (error: %ld).\n"), retval);
+		exit(1);
 	}
 
 	setbuf(stdout, NULL);
 	setbuf(stderr, NULL);
-	add_error_table(&et_ext2_error_table);
-	add_error_table(&et_prof_error_table);
+	retval = add_error_table(&et_ext2_error_table);
+	if (!retval)
+		retval = add_error_table(&et_prof_error_table);
+	if (retval) {
+		fprintf(stderr, _("Unable to add error information.\n"));
+		exit(1);
+	}
 	memset(&fs_param, 0, sizeof(struct ext2_super_block));
 	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
 
-- 
1.7.0.4

[PATCH 13/15] mke2fs: fix potential memory leak in mke2fs_setup_tdb()

[Namhyung Kim]

'tmp_name' allocated by strdup() should also be freed if error.
Also check return value of set_undo_io_backup_file() for possible
memory failure. A whitespace fix is added too.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 6e2092d..644b287 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1882,15 +1882,17 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 
 	tmp_name = strdup(name);
 	if (!tmp_name) {
-	alloc_fn_fail:
-		com_err(program_name, ENOMEM, 
+alloc_fn_fail:
+		com_err(program_name, ENOMEM,
 			_("Couldn't allocate memory for tdb filename\n"));
 		return ENOMEM;
 	}
 	device_name = basename(tmp_name);
 	tdb_file = malloc(strlen(tdb_dir) + 8 + strlen(device_name) + 7 + 1);
-	if (!tdb_file)
+	if (!tdb_file) {
+		free(tmp_name);
 		goto alloc_fn_fail;
+	}
 	sprintf(tdb_file, "%s/mke2fs-%s.e2undo", tdb_dir, device_name);
 
 	if (!access(tdb_file, F_OK)) {
@@ -1899,6 +1901,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 			com_err(program_name, retval,
 				_("while trying to delete %s"),
 				tdb_file);
+			free(tmp_name);
 			free(tdb_file);
 			return retval;
 		}
@@ -1906,7 +1909,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 
 	set_undo_io_backing_manager(*io_ptr);
 	*io_ptr = undo_io_manager;
-	set_undo_io_backup_file(tdb_file);
+	retval = set_undo_io_backup_file(tdb_file);
 	printf(_("Overwriting existing filesystem; this can be undone "
 		 "using the command:\n"
 		 "    e2undo %s %s\n\n"), tdb_file, name);
-- 
1.7.0.4

[PATCH 14/15] libext2fs: fix possible memory leak in write_journal_inode()

[Namhyung Kim]

ext2fs_zero_block2() allocates static buffer if needed so it
should be freed at last (call it again with 0 args).

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 lib/ext2fs/mkjournal.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
index 9466e78..242c537 100644
--- a/lib/ext2fs/mkjournal.c
+++ b/lib/ext2fs/mkjournal.c
@@ -376,6 +376,7 @@ static errcode_t write_journal_inode(ext2_filsys fs, ext2_ino_t journal_ino,
 	ext2fs_mark_super_dirty(fs);
 
 errout:
+	ext2fs_zero_blocks2(0, 0, 0, 0, 0);
 	ext2fs_free_mem(&buf);
 	return retval;
 }
-- 
1.7.0.4

[PATCH 15/15] mke2fs.8.in: add ENVIRONMENT section

[Namhyung Kim]

Add ENVIRONMENT section and describe behavior of MKE2FS_SYNC,
MKE2FS_CONFIG, MKE2FS_FIRST_META_BG, MKE2FS_DEVICE_SECTSIZE
and MKE2FS_SKIP_CHECK_MSG.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.8.in |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index 548ef66..e776dcf 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -642,6 +642,29 @@ Verbose execution.
 Print the version number of
 .B mke2fs
 and exit.
+.SH ENVIRONMENT
+.TP
+.BI MKE2FS_SYNC
+If set to non-zero integer value, its value is used to determine how often
+.BR sync (2)
+is called during inode table initialization.
+.TP
+.BI MKE2FS_CONFIG
+Determines the location of the configuration file (see
+.BR mke2fs.conf (5)).
+.TP
+.BI MKE2FS_FIRST_META_BG
+If set to non-zero integer value, its value is used to determine first meta
+block group. This is mostly for debugging purposes.
+.TP
+.BI MKE2FS_DEVICE_SECTSIZE
+If set to non-zero integer value, its value is used to determine physical
+sector size of the
+.IR device .
+.TP
+.BI MKE2FS_SKIP_CHECK_MSG
+If set, do not show the message of filesystem automatic check caused by
+mount count or check interval.
 .SH AUTHOR
 This version of
 .B mke2fs
-- 
1.7.0.4

Re: [PATCH 03/15] mke2fs: simplify inode table block counting

[Lukas Czerner]

On Mon, 29 Nov 2010, Namhyung Kim wrote:

> Simplify counting of inode table blocks in a block group using
> local variable 'ipb'. Otherwise the variable could be removed
> because there was no user of it.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 0980045..9fb5d5f 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -320,11 +320,8 @@ static void write_inode_tables(ext2_filsys fs, int lazy_flag, int itable_zeroed)
>  
>  		if (lazy_flag) {
>  			ipb = fs->blocksize / EXT2_INODE_SIZE(fs->super);
> -			num = ((((fs->super->s_inodes_per_group -
> -				  ext2fs_bg_itable_unused(fs, i)) *
> -				 EXT2_INODE_SIZE(fs->super)) +
> -				EXT2_BLOCK_SIZE(fs->super) - 1) /
> -			       EXT2_BLOCK_SIZE(fs->super));
> +			num = (fs->super->s_inodes_per_group -
> +			       ext2fs_bg_itable_unused(fs, i) + ipb - 1) / ipb;
>  		}
>  		if (!lazy_flag || itable_zeroed) {
>  			/* The kernel doesn't need to zero the itable blocks */
> 

Hi,

I would rather add this macro into header file (lib/ext2fs/ext2fs.h
maybe?)

#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

remoevd ipb variable because it is rather useless. And do something like
this ?

num = DIV_ROUND_UP((fs->super->s_inodes_per_group -
		    ext2fs_bg_itable_unused(fs, i)) *
		   EXT2_INODE_SIZE(fs->super), 
		   EXT2_BLOCK_SIZE(fs->super));

IMO it improves readability a lot and I am sure that there are other
places which may take advantage of the DIV_ROUND_UP macro.

Thanks!

-Lukas

Re: [PATCH 08/15] libext2fs: fix memory leak on error path

[Lukas Czerner]

On Mon, 29 Nov 2010, Namhyung Kim wrote:

> io->name should be freed if ext2fs_file_open2() fails.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  lib/ext2fs/inode_io.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/ext2fs/inode_io.c b/lib/ext2fs/inode_io.c
> index 4faaa48..bc934d3 100644
> --- a/lib/ext2fs/inode_io.c
> +++ b/lib/ext2fs/inode_io.c
> @@ -157,11 +157,13 @@ static errcode_t inode_open(const char *name, int flags, io_channel *channel)
>  				   &data->inode : 0, open_flags,
>  				   &data->file);
>  	if (retval)
> -		goto cleanup;
> +		goto cleanup_name;
>  
>  	*channel = io;
>  	return 0;
>  
> +cleanup_name:
> +	ext2fs_free_mem(&io->name);
>  cleanup:
>  	if (data) {
>  		ext2fs_free_mem(&data);
> 

Hmm, are those check-on-free everywhere really necessary ? Would not
make more sense to check it in ext2fs_free_mem and then when we hit
things like this patch is trying to fix, just remove the conditions ?

I am not suggesting anything like "go through all e2fsprogs and change
the conditions on free", but really change it when we poke the code anyway.

-Lukas

Re: [PATCH 11/15] mke2fs: fix determination of size_type

[Lukas Czerner]

On Mon, 29 Nov 2010, Namhyung Kim wrote:

> In original code, 'huge' type could not be selected because it
> always be caught for 'big' type. Change the ordering.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 90cc206..b88decf 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -947,10 +947,10 @@ static char **parse_fs_type(const char *fs_type,
>  		size_type = "floppy";
>  	else if (fs_blocks_count < 512 * meg)
>  		size_type = "small";
> -	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> -		size_type = "big";
>  	else if (fs_blocks_count >= 16 * 1024 * 1024 * meg)
>  		size_type = "huge";
> +	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> +		size_type = "big";
>  	else
>  		size_type = "default";
>  
> 

Personally, I do not like it very much. Either sort this in ascending order
and use "<" or sort it in descending order and use ">=". But since it
was originally sorted in ascending order I would do that this way.

-Lukas

Re: [PATCH 12/15] mke2fs: add some error checks into PRS()

[Lukas Czerner]

On Mon, 29 Nov 2010, Namhyung Kim wrote:

> Check return value of some functions and exit if unhandled error occurred.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index b88decf..6e2092d 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1108,6 +1108,10 @@ static void PRS(int argc, char *argv[])
>  	if (oldpath)
>  		pathlen += strlen(oldpath);
>  	newpath = malloc(pathlen);
> +	if (!newpath) {
> +		fprintf(stderr, _("Couldn't allocate memory for new PATH.\n"));
> +		exit(1);
> +	}
>  	strcpy(newpath, PATH_SET);
>  
>  	/* Update our PATH to include /sbin  */
> @@ -1138,14 +1142,28 @@ static void PRS(int argc, char *argv[])
>  	profile_set_syntax_err_cb(syntax_err_report);
>  	retval = profile_init(config_fn, &profile);
>  	if (retval == ENOENT) {
> -		profile_init(default_files, &profile);
> -		profile_set_default(profile, mke2fs_default_profile);
> +		retval = profile_init(default_files, &profile);
> +		if (retval)
> +			goto profile_error;
> +		retval = profile_set_default(profile, mke2fs_default_profile);
> +		if (retval)
> +			goto profile_error;
> +	} else {

Maybe use "else if (retval)" since profile_init(config_fn, &profile);
might exit successfully (return 0) ?

> +profile_error:
> +		fprintf(stderr, _("Couldn't init profile successfully"
> +				  " (error: %ld).\n"), retval);
> +		exit(1);
>  	}
>  
>  	setbuf(stdout, NULL);
>  	setbuf(stderr, NULL);
> -	add_error_table(&et_ext2_error_table);
> -	add_error_table(&et_prof_error_table);
> +	retval = add_error_table(&et_ext2_error_table);
> +	if (!retval)
> +		retval = add_error_table(&et_prof_error_table);
> +	if (retval) {
> +		fprintf(stderr, _("Unable to add error information.\n"));
> +		exit(1);
> +	}
>  	memset(&fs_param, 0, sizeof(struct ext2_super_block));
>  	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
>  
> 

-Lukas

Re: [PATCH 13/15] mke2fs: fix potential memory leak in mke2fs_setup_tdb()

[Lukas Czerner]

On Mon, 29 Nov 2010, Namhyung Kim wrote:

> 'tmp_name' allocated by strdup() should also be freed if error.
> Also check return value of set_undo_io_backup_file() for possible
> memory failure. A whitespace fix is added too.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 6e2092d..644b287 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1882,15 +1882,17 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  
>  	tmp_name = strdup(name);
>  	if (!tmp_name) {
> -	alloc_fn_fail:
> -		com_err(program_name, ENOMEM, 
> +alloc_fn_fail:
> +		com_err(program_name, ENOMEM,
>  			_("Couldn't allocate memory for tdb filename\n"));
>  		return ENOMEM;
>  	}

What about putting the alloc_fn_fail at the end of the function ? after return
retval?

>  	device_name = basename(tmp_name);
>  	tdb_file = malloc(strlen(tdb_dir) + 8 + strlen(device_name) + 7 + 1);
> -	if (!tdb_file)
> +	if (!tdb_file) {
> +		free(tmp_name);

What about adding

if (tmp_name)
	free(tmp_name);

in alloc_fs_fail context ?

>  		goto alloc_fn_fail;
> +	}
>  	sprintf(tdb_file, "%s/mke2fs-%s.e2undo", tdb_dir, device_name);
>  
>  	if (!access(tdb_file, F_OK)) {
> @@ -1899,6 +1901,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  			com_err(program_name, retval,
>  				_("while trying to delete %s"),
>  				tdb_file);
> +			free(tmp_name);
>  			free(tdb_file);
>  			return retval;
>  		}
> @@ -1906,7 +1909,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  
>  	set_undo_io_backing_manager(*io_ptr);
>  	*io_ptr = undo_io_manager;
> -	set_undo_io_backup_file(tdb_file);
> +	retval = set_undo_io_backup_file(tdb_file);

You should probably return ENOMEM when this fails, moreover when
set_undo_io_backup() you'll try to free not allocated space.

>  	printf(_("Overwriting existing filesystem; this can be undone "
>  		 "using the command:\n"
>  		 "    e2undo %s %s\n\n"), tdb_file, name);
> 

Thanks
-Lukas

Re: [PATCH 03/15] mke2fs: simplify inode table block counting

[Namhyung Kim]

2010-11-30 (화), 13:01 +0100, Lukas Czerner:
> On Mon, 29 Nov 2010, Namhyung Kim wrote:
> 
> > Simplify counting of inode table blocks in a block group using
> > local variable 'ipb'. Otherwise the variable could be removed
> > because there was no user of it.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > ---
> >  misc/mke2fs.c |    7 ++-----
> >  1 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index 0980045..9fb5d5f 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -320,11 +320,8 @@ static void write_inode_tables(ext2_filsys fs, int lazy_flag, int itable_zeroed)
> >  
> >  		if (lazy_flag) {
> >  			ipb = fs->blocksize / EXT2_INODE_SIZE(fs->super);
> > -			num = ((((fs->super->s_inodes_per_group -
> > -				  ext2fs_bg_itable_unused(fs, i)) *
> > -				 EXT2_INODE_SIZE(fs->super)) +
> > -				EXT2_BLOCK_SIZE(fs->super) - 1) /
> > -			       EXT2_BLOCK_SIZE(fs->super));
> > +			num = (fs->super->s_inodes_per_group -
> > +			       ext2fs_bg_itable_unused(fs, i) + ipb - 1) / ipb;
> >  		}
> >  		if (!lazy_flag || itable_zeroed) {
> >  			/* The kernel doesn't need to zero the itable blocks */
> > 
> 
> Hi,
> 
> I would rather add this macro into header file (lib/ext2fs/ext2fs.h
> maybe?)
> 
> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> 
> remoevd ipb variable because it is rather useless. And do something like
> this ?
> 
> num = DIV_ROUND_UP((fs->super->s_inodes_per_group -
> 		    ext2fs_bg_itable_unused(fs, i)) *
> 		   EXT2_INODE_SIZE(fs->super), 
> 		   EXT2_BLOCK_SIZE(fs->super));
> 
> IMO it improves readability a lot and I am sure that there are other
> places which may take advantage of the DIV_ROUND_UP macro.
> 
> Thanks!
> 
> -Lukas

Hi, Lukas.
Thanks for the review.

We have ext2fs_div[64]_ceil() for that. So I can use it like

ipb = fs->blocksize / EXT2_INODE_SIZE(fs->super);
num = ext2_fs_div_ceil(fs->super->s_inodes_per_group -
			ext2fs_bg_itable_unused(fs, i), ipb);

or

num = ext2_fs_div_ceil((fs->super->s_inodes_per_group -
			ext2fs_bg_itable_unused(fs, i)) *
		       EXT2_INODE_SIZE(fs->super),
		       EXT2_BLOCK_SIZE(fs->super));

Either is fine to me. Ted, what's your preference?

-- 
Regards,
Namhyung Kim


--
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 12/15] mke2fs: add some error checks into PRS()

[Namhyung Kim]

2010-11-30 (화), 13:46 +0100, Lukas Czerner:
> On Mon, 29 Nov 2010, Namhyung Kim wrote:
> 
> > Check return value of some functions and exit if unhandled error occurred.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > ---
> >  misc/mke2fs.c |   26 ++++++++++++++++++++++----
> >  1 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index b88decf..6e2092d 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -1108,6 +1108,10 @@ static void PRS(int argc, char *argv[])
> >  	if (oldpath)
> >  		pathlen += strlen(oldpath);
> >  	newpath = malloc(pathlen);
> > +	if (!newpath) {
> > +		fprintf(stderr, _("Couldn't allocate memory for new PATH.\n"));
> > +		exit(1);
> > +	}
> >  	strcpy(newpath, PATH_SET);
> >  
> >  	/* Update our PATH to include /sbin  */
> > @@ -1138,14 +1142,28 @@ static void PRS(int argc, char *argv[])
> >  	profile_set_syntax_err_cb(syntax_err_report);
> >  	retval = profile_init(config_fn, &profile);
> >  	if (retval == ENOENT) {
> > -		profile_init(default_files, &profile);
> > -		profile_set_default(profile, mke2fs_default_profile);
> > +		retval = profile_init(default_files, &profile);
> > +		if (retval)
> > +			goto profile_error;
> > +		retval = profile_set_default(profile, mke2fs_default_profile);
> > +		if (retval)
> > +			goto profile_error;
> > +	} else {
> 
> Maybe use "else if (retval)" since profile_init(config_fn, &profile);
> might exit successfully (return 0) ?
> 

Right! Will resend v2, thanks.

-- 
Regards,
Namhyung Kim


--
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 13/15] mke2fs: fix potential memory leak in mke2fs_setup_tdb()

[Namhyung Kim]

2010-11-30 (화), 14:02 +0100, Lukas Czerner:
> On Mon, 29 Nov 2010, Namhyung Kim wrote:
> 
> > 'tmp_name' allocated by strdup() should also be freed if error.
> > Also check return value of set_undo_io_backup_file() for possible
> > memory failure. A whitespace fix is added too.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > ---
> >  misc/mke2fs.c |   11 +++++++----
> >  1 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index 6e2092d..644b287 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -1882,15 +1882,17 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
> >  
> >  	tmp_name = strdup(name);
> >  	if (!tmp_name) {
> > -	alloc_fn_fail:
> > -		com_err(program_name, ENOMEM, 
> > +alloc_fn_fail:
> > +		com_err(program_name, ENOMEM,
> >  			_("Couldn't allocate memory for tdb filename\n"));
> >  		return ENOMEM;
> >  	}
> 
> What about putting the alloc_fn_fail at the end of the function ? after return
> retval?
> 

No problem.


> >  	device_name = basename(tmp_name);
> >  	tdb_file = malloc(strlen(tdb_dir) + 8 + strlen(device_name) + 7 + 1);
> > -	if (!tdb_file)
> > +	if (!tdb_file) {
> > +		free(tmp_name);
> 
> What about adding
> 
> if (tmp_name)
> 	free(tmp_name);
> 
> in alloc_fs_fail context ?

I guess just free(tmp_name) is enough.


> >  		goto alloc_fn_fail;
> > +	}
> >  	sprintf(tdb_file, "%s/mke2fs-%s.e2undo", tdb_dir, device_name);
> >  
> >  	if (!access(tdb_file, F_OK)) {
> > @@ -1899,6 +1901,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
> >  			com_err(program_name, retval,
> >  				_("while trying to delete %s"),
> >  				tdb_file);
> > +			free(tmp_name);
> >  			free(tdb_file);
> >  			return retval;
> >  		}
> > @@ -1906,7 +1909,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
> >  
> >  	set_undo_io_backing_manager(*io_ptr);
> >  	*io_ptr = undo_io_manager;
> > -	set_undo_io_backup_file(tdb_file);
> > +	retval = set_undo_io_backup_file(tdb_file);
> 
> You should probably return ENOMEM when this fails, moreover when
> set_undo_io_backup() you'll try to free not allocated space.
> 

Its only user doesn't check the actual return value and simply does
exit(1). And confusingly, tdb_file here is a local variable and
allocated successully so there will be no problem. Also AFAIK C permits
to pass NULL to free(), means no operation.

-- 
Regards,
Namhyung Kim


--
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 11/15] mke2fs: fix determination of size_type

[Namhyung Kim]

2010-11-30 (화), 13:33 +0100, Lukas Czerner:
> On Mon, 29 Nov 2010, Namhyung Kim wrote:
> 
> > In original code, 'huge' type could not be selected because it
> > always be caught for 'big' type. Change the ordering.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > ---
> >  misc/mke2fs.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index 90cc206..b88decf 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -947,10 +947,10 @@ static char **parse_fs_type(const char *fs_type,
> >  		size_type = "floppy";
> >  	else if (fs_blocks_count < 512 * meg)
> >  		size_type = "small";
> > -	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> > -		size_type = "big";
> >  	else if (fs_blocks_count >= 16 * 1024 * 1024 * meg)
> >  		size_type = "huge";
> > +	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> > +		size_type = "big";
> >  	else
> >  		size_type = "default";
> >  
> > 
> 
> Personally, I do not like it very much. Either sort this in ascending order
> and use "<" or sort it in descending order and use ">=". But since it
> was originally sorted in ascending order I would do that this way.
> 
> -Lukas

No problem. You mean "floppy, small, default, big and huge", right? I'll
resend v2 if you prefer that, Ted.

Thanks.


-- 
Regards,
Namhyung Kim


--
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 11/15] mke2fs: fix determination of size_type

[Lukas Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1481 bytes --]

On Wed, 1 Dec 2010, Namhyung Kim wrote:

> 2010-11-30 (화), 13:33 +0100, Lukas Czerner:
> > On Mon, 29 Nov 2010, Namhyung Kim wrote:
> > 
> > > In original code, 'huge' type could not be selected because it
> > > always be caught for 'big' type. Change the ordering.
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > > ---
> > >  misc/mke2fs.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > > index 90cc206..b88decf 100644
> > > --- a/misc/mke2fs.c
> > > +++ b/misc/mke2fs.c
> > > @@ -947,10 +947,10 @@ static char **parse_fs_type(const char *fs_type,
> > >  		size_type = "floppy";
> > >  	else if (fs_blocks_count < 512 * meg)
> > >  		size_type = "small";
> > > -	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> > > -		size_type = "big";
> > >  	else if (fs_blocks_count >= 16 * 1024 * 1024 * meg)
> > >  		size_type = "huge";
> > > +	else if (fs_blocks_count >= 4 * 1024 * 1024 * meg)
> > > +		size_type = "big";
> > >  	else
> > >  		size_type = "default";
> > >  
> > > 
> > 
> > Personally, I do not like it very much. Either sort this in ascending order
> > and use "<" or sort it in descending order and use ">=". But since it
> > was originally sorted in ascending order I would do that this way.
> > 
> > -Lukas
> 
> No problem. You mean "floppy, small, default, big and huge", right? I'll
> resend v2 if you prefer that, Ted.
> 
> Thanks.
> 

Right.

Thanks!

-Lukas

[PATCH v2 12/15] mke2fs: add some error checks into PRS()

[Namhyung Kim]

Check return value of some functions and exit if unhandled error occurred.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index b88decfc2f27..6e2092dc051e 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1108,6 +1108,10 @@ static void PRS(int argc, char *argv[])
 	if (oldpath)
 		pathlen += strlen(oldpath);
 	newpath = malloc(pathlen);
+	if (!newpath) {
+		fprintf(stderr, _("Couldn't allocate memory for new PATH.\n"));
+		exit(1);
+	}
 	strcpy(newpath, PATH_SET);
 
 	/* Update our PATH to include /sbin  */
@@ -1138,14 +1142,28 @@ static void PRS(int argc, char *argv[])
 	profile_set_syntax_err_cb(syntax_err_report);
 	retval = profile_init(config_fn, &profile);
 	if (retval == ENOENT) {
-		profile_init(default_files, &profile);
-		profile_set_default(profile, mke2fs_default_profile);
+		retval = profile_init(default_files, &profile);
+		if (retval)
+			goto profile_error;
+		retval = profile_set_default(profile, mke2fs_default_profile);
+		if (retval)
+			goto profile_error;
+	} else if (retval) {
+profile_error:
+		fprintf(stderr, _("Couldn't init profile successfully"
+				  " (error: %ld).\n"), retval);
+		exit(1);
 	}
 
 	setbuf(stdout, NULL);
 	setbuf(stderr, NULL);
-	add_error_table(&et_ext2_error_table);
-	add_error_table(&et_prof_error_table);
+	retval = add_error_table(&et_ext2_error_table);
+	if (!retval)
+		retval = add_error_table(&et_prof_error_table);
+	if (retval) {
+		fprintf(stderr, _("Unable to add error information.\n"));
+		exit(1);
+	}
 	memset(&fs_param, 0, sizeof(struct ext2_super_block));
 	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
 
-- 
1.7.3.3.400.g93cef

[PATCH v2 13/15] mke2fs: fix potential memory leak in mke2fs_setup_tdb()

[Namhyung Kim]

'tmp_name' allocated by strdup() should also be freed if error.
Also check return value of set_undo_io_backup_file() for possible
memory failure. And move label 'alloc_fn_fail' to the end of the
function.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 misc/mke2fs.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 6e2092dc051e..2150f62c3e4c 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1881,12 +1881,8 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 		return 0;
 
 	tmp_name = strdup(name);
-	if (!tmp_name) {
-	alloc_fn_fail:
-		com_err(program_name, ENOMEM, 
-			_("Couldn't allocate memory for tdb filename\n"));
-		return ENOMEM;
-	}
+	if (!tmp_name)
+		goto alloc_fn_fail;
 	device_name = basename(tmp_name);
 	tdb_file = malloc(strlen(tdb_dir) + 8 + strlen(device_name) + 7 + 1);
 	if (!tdb_file)
@@ -1899,6 +1895,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 			com_err(program_name, retval,
 				_("while trying to delete %s"),
 				tdb_file);
+			free(tmp_name);
 			free(tdb_file);
 			return retval;
 		}
@@ -1906,7 +1903,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 
 	set_undo_io_backing_manager(*io_ptr);
 	*io_ptr = undo_io_manager;
-	set_undo_io_backup_file(tdb_file);
+	retval = set_undo_io_backup_file(tdb_file);
 	printf(_("Overwriting existing filesystem; this can be undone "
 		 "using the command:\n"
 		 "    e2undo %s %s\n\n"), tdb_file, name);
@@ -1914,6 +1911,12 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 	free(tdb_file);
 	free(tmp_name);
 	return retval;
+
+alloc_fn_fail:
+	free(tmp_name);
+	com_err(program_name, ENOMEM,
+		_("Couldn't allocate memory for tdb filename\n"));
+	return ENOMEM;
 }
 
 #ifdef __linux__
-- 
1.7.3.3.400.g93cef

Re: [PATCH v2 12/15] mke2fs: add some error checks into PRS()

[Lukas Czerner]

On Thu, 16 Dec 2010, Namhyung Kim wrote:

> Check return value of some functions and exit if unhandled error occurred.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index b88decfc2f27..6e2092dc051e 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1108,6 +1108,10 @@ static void PRS(int argc, char *argv[])
>  	if (oldpath)
>  		pathlen += strlen(oldpath);
>  	newpath = malloc(pathlen);
> +	if (!newpath) {
> +		fprintf(stderr, _("Couldn't allocate memory for new PATH.\n"));
> +		exit(1);
> +	}
>  	strcpy(newpath, PATH_SET);
>  
>  	/* Update our PATH to include /sbin  */
> @@ -1138,14 +1142,28 @@ static void PRS(int argc, char *argv[])
>  	profile_set_syntax_err_cb(syntax_err_report);
>  	retval = profile_init(config_fn, &profile);
>  	if (retval == ENOENT) {
> -		profile_init(default_files, &profile);
> -		profile_set_default(profile, mke2fs_default_profile);
> +		retval = profile_init(default_files, &profile);
> +		if (retval)
> +			goto profile_error;
> +		retval = profile_set_default(profile, mke2fs_default_profile);
> +		if (retval)
> +			goto profile_error;
> +	} else if (retval) {
> +profile_error:
> +		fprintf(stderr, _("Couldn't init profile successfully"
> +				  " (error: %ld).\n"), retval);
> +		exit(1);
>  	}
>  
>  	setbuf(stdout, NULL);
>  	setbuf(stderr, NULL);
> -	add_error_table(&et_ext2_error_table);
> -	add_error_table(&et_prof_error_table);
> +	retval = add_error_table(&et_ext2_error_table);
> +	if (!retval)
> +		retval = add_error_table(&et_prof_error_table);
> +	if (retval) {
> +		fprintf(stderr, _("Unable to add error information.\n"));
> +		exit(1);
> +	}
>  	memset(&fs_param, 0, sizeof(struct ext2_super_block));
>  	fs_param.s_rev_level = 1;  /* Create revision 1 filesystems now */
>  
> 

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!

-Lukas

Re: [PATCH v2 13/15] mke2fs: fix potential memory leak in mke2fs_setup_tdb()

[Lukas Czerner]

On Thu, 16 Dec 2010, Namhyung Kim wrote:

> 'tmp_name' allocated by strdup() should also be freed if error.
> Also check return value of set_undo_io_backup_file() for possible
> memory failure. And move label 'alloc_fn_fail' to the end of the
> function.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  misc/mke2fs.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 6e2092dc051e..2150f62c3e4c 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1881,12 +1881,8 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  		return 0;
>  
>  	tmp_name = strdup(name);
> -	if (!tmp_name) {
> -	alloc_fn_fail:
> -		com_err(program_name, ENOMEM, 
> -			_("Couldn't allocate memory for tdb filename\n"));
> -		return ENOMEM;
> -	}
> +	if (!tmp_name)
> +		goto alloc_fn_fail;
>  	device_name = basename(tmp_name);
>  	tdb_file = malloc(strlen(tdb_dir) + 8 + strlen(device_name) + 7 + 1);
>  	if (!tdb_file)
> @@ -1899,6 +1895,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  			com_err(program_name, retval,
>  				_("while trying to delete %s"),
>  				tdb_file);
> +			free(tmp_name);
>  			free(tdb_file);
>  			return retval;
>  		}
> @@ -1906,7 +1903,7 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  
>  	set_undo_io_backing_manager(*io_ptr);
>  	*io_ptr = undo_io_manager;
> -	set_undo_io_backup_file(tdb_file);
> +	retval = set_undo_io_backup_file(tdb_file);
>  	printf(_("Overwriting existing filesystem; this can be undone "
>  		 "using the command:\n"
>  		 "    e2undo %s %s\n\n"), tdb_file, name);
> @@ -1914,6 +1911,12 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  	free(tdb_file);
>  	free(tmp_name);
>  	return retval;
> +
> +alloc_fn_fail:
> +	free(tmp_name);
> +	com_err(program_name, ENOMEM,
> +		_("Couldn't allocate memory for tdb filename\n"));
> +	return ENOMEM;
>  }
>  
>  #ifdef __linux__
> 

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!

-Lukas

Re: [01/15, RESEND] libext2fs: fix potential build failure with OMIT_COM_ERR

[Ted Ts'o]

On Sun, Nov 28, 2010 at 10:55:03PM -0000, Namhyung Kim wrote:
> This fixes following build failure when OMIT_COM_ERR is defined:
> 
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_clear_generic_bitmap’:
>  lib/ext2fs/gen_bitmap.c:437: error: invalid storage class for function ‘ext2fs_test_clear_generic_bitmap_range’
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_get_generic_bitmap_end’:
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_get_generic_bitmap_start’:
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_unmark_generic_bitmap’:
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_mark_generic_bitmap’:
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
>  lib/ext2fs/gen_bitmap.c: In function ‘ext2fs_test_generic_bitmap’:
>  lib/ext2fs/gen_bitmap.c:559: error: expected declaration or statement at end of input
> make[2]: *** [gen_bitmap.o] Error 1
> make[2]: Leaving directory e2fsprogs/lib/ext2fs'
> make[1]: *** [all-libs-recursive] Error 1
> make[1]: Leaving directory e2fsprogs'
> make: *** [all] Error 2
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Applied, thanks.

					- 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

Re: [02/15, RESEND] libext2fs: fix dubious code in ext2fs_numeric_progress_init()

[Ted Ts'o]

On Sun, Nov 28, 2010 at 10:55:04PM -0000, Namhyung Kim wrote:
> Looks like a copy & paste problem to me.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Applied, thanks.

					- Ted

Re: [PATCH 03/15] mke2fs: simplify inode table block counting

[Ted Ts'o]

On Wed, Dec 01, 2010 at 08:49:05PM +0900, Namhyung Kim wrote:
> 
> We have ext2fs_div[64]_ceil() for that. So I can use it like
> 
> ipb = fs->blocksize / EXT2_INODE_SIZE(fs->super);
> num = ext2_fs_div_ceil(fs->super->s_inodes_per_group -
> 			ext2fs_bg_itable_unused(fs, i), ipb);
> 
> or
> 
> num = ext2_fs_div_ceil((fs->super->s_inodes_per_group -
> 			ext2fs_bg_itable_unused(fs, i)) *
> 		       EXT2_INODE_SIZE(fs->super),
> 		       EXT2_BLOCK_SIZE(fs->super));
> 
> Either is fine to me. Ted, what's your preference?

I've checked in the second since it's closer to the original code.

     	     	    	   	      - Ted

Re: [PATCH 04/15] libext2fs: remove unnecessary casts to ext2fs_generic_bitmap

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:06PM +0900, Namhyung Kim wrote:
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Applied, thanks

					- Ted

Re: [PATCH 05/15] libext2fs: fix dubious code in ext2fs_unmark_generic_bitmap()

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:07PM +0900, Namhyung Kim wrote:
> Looks like a copy & paste problem to me.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Applied, thanks.

					- Ted

Re: [PATCH 06/15] libext2fs: invalid EXT4_FEATURE_RO_COMPAT_HUGE_FILE checks

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:08PM +0900, Namhyung Kim wrote:
> It should be checked with fs->super->s_feature_ro_compat instead of
> ->s_feature_incompat.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

A similar patch has already been applied, thanks.

  	  	    	    	 	  - Ted

Re: [PATCH 07/15] libext2fs: fix error path in ext2fs_update_bb_inode()

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:09PM +0900, Namhyung Kim wrote:
> If ext2fs_get_mem() on rec.block_buf fails we should not call
> ext2fs_free_mem() on it.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks for pointing this out.  I fixed this in a slightly simpler way.

       	   	    	       	       	    - Ted

commit 2150278fa25f3fe8b8f29835ccd3079b608bb825
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Mon Dec 20 10:57:29 2010 -0500

    libext2fs: fix potential free() of garbage in ext2fs_update_bb_inode()
    
    There was a potential of freeing an uninitialized pointer in
    rec.block_buf, which was pointed out by Namhyung Kim <namhyung@gmail.com>
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/lib/ext2fs/bb_inode.c b/lib/ext2fs/bb_inode.c
index 0b79b16..0b6c3dd 100644
--- a/lib/ext2fs/bb_inode.c
+++ b/lib/ext2fs/bb_inode.c
@@ -65,8 +65,7 @@ errcode_t ext2fs_update_bb_inode(ext2_filsys fs, ext2_badblocks_list bb_list)
 	if (!fs->block_map)
 		return EXT2_ET_NO_BLOCK_BITMAP;
 
-	rec.bad_block_count = 0;
-	rec.ind_blocks_size = rec.ind_blocks_ptr = 0;
+	memset(&rec, 0, sizeof(rec));
 	rec.max_ind_blocks = 10;
 	retval = ext2fs_get_array(rec.max_ind_blocks, sizeof(blk_t),
 				&rec.ind_blocks);

Re: [PATCH 08/15] libext2fs: fix memory leak on error path

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:10PM +0900, Namhyung Kim wrote:
> io->name should be freed if ext2fs_file_open2() fails.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks, fixed (although in a slightly different way to avoid the extra
goto label).

					- Ted

Re: [PATCH 09/15] mke2fs: check return value of e2p_os2string()

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:11PM +0900, Namhyung Kim wrote:
> e2p_os2string() calls malloc() so that it can return NULL.
> Check it.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks for reporting this, I fixed this problem in a slightly
different way.

		       	    	     - Ted

Re: [PATCH 10/15] mke2fs.8.in: add missing "big" and "huge" usage-type description

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:12PM +0900, Namhyung Kim wrote:
> The commit 493024ea1d74e4cb48aac3a24111f5c8da343e9f ("mke2fs: Fix up the
> fs type and feature selection for 64-bit blocks") added 'big' and 'huge'
> usage-type but was missing description in man page. Add it.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks, applied.

					- Ted

Re: [PATCH 11/15] mke2fs: fix determination of size_type

[Ted Ts'o]

On Wed, Dec 01, 2010 at 04:46:20PM +0100, Lukas Czerner wrote:
> > 
> > No problem. You mean "floppy, small, default, big and huge", right? I'll
> > resend v2 if you prefer that, Ted.

That's what I've checked into the next branch, thanks.

       	    	 	      	       	       - Ted

Re: [PATCH v2 12/15] mke2fs: add some error checks into PRS()

[Ted Ts'o]

On Thu, Dec 16, 2010 at 06:40:40PM +0900, Namhyung Kim wrote:
> Check return value of some functions and exit if unhandled error occurred.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

It's actually ok to continue if the error tables aren't initialized,
so we don't need to add error checking for them.  I've add the patch
(minus this hunk) to the next branch.

      	    	    	      	       	   - Ted

Re: [PATCH 14/15] libext2fs: fix possible memory leak in write_journal_inode()

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:16PM +0900, Namhyung Kim wrote:
> ext2fs_zero_block2() allocates static buffer if needed so it
> should be freed at last (call it again with 0 args).
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks, applied.

						- Ted

Re: [PATCH 15/15] mke2fs.8.in: add ENVIRONMENT section

[Ted Ts'o]

On Mon, Nov 29, 2010 at 05:55:17PM +0900, Namhyung Kim wrote:
> Add ENVIRONMENT section and describe behavior of MKE2FS_SYNC,
> MKE2FS_CONFIG, MKE2FS_FIRST_META_BG, MKE2FS_DEVICE_SECTSIZE
> and MKE2FS_SKIP_CHECK_MSG.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

Thanks, applied to the 'next' branch.

					- Ted