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(-)
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 <[email protected]>
---
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
Looks like a copy & paste problem to me.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Namhyung Kim <[email protected]>
---
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
Looks like a copy & paste problem to me.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
It should be checked with fs->super->s_feature_ro_compat instead of
->s_feature_incompat.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
If ext2fs_get_mem() on rec.block_buf fails we should not call
ext2fs_free_mem() on it.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
---
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
io->name should be freed if ext2fs_file_open2() fails.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
---
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
'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 <[email protected]>
---
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
e2p_os2string() calls malloc() so that it can return NULL.
Check it.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
Check return value of some functions and exit if unhandled error occurred.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Mon, 29 Nov 2010, Namhyung Kim wrote:
> io->name should be freed if ext2fs_file_open2() fails.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
Check return value of some functions and exit if unhandled error occurred.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
'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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
-Lukas
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
-Lukas
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 <[email protected]>
Applied, thanks.
- Ted
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 <[email protected]>
Applied, thanks.
- Ted
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
On Mon, Nov 29, 2010 at 05:55:06PM +0900, Namhyung Kim wrote:
> Signed-off-by: Namhyung Kim <[email protected]>
Applied, thanks
- Ted
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 <[email protected]>
Applied, thanks.
- Ted
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 <[email protected]>
A similar patch has already been applied, thanks.
- Ted
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 <[email protected]>
Thanks for pointing this out. I fixed this in a slightly simpler way.
- Ted
commit 2150278fa25f3fe8b8f29835ccd3079b608bb825
Author: Theodore Ts'o <[email protected]>
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 <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
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);
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 <[email protected]>
Thanks, fixed (although in a slightly different way to avoid the extra
goto label).
- Ted
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 <[email protected]>
Thanks for reporting this, I fixed this problem in a slightly
different way.
- Ted
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 <[email protected]>
Thanks, applied.
- Ted
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
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 <[email protected]>
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
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 <[email protected]>
Thanks, applied.
- Ted
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 <[email protected]>
Thanks, applied to the 'next' branch.
- Ted