* [PATCH] ext4: fix error handling in ext4_fill_super() @ 2012-10-08 11:09 Eugene Shatokhin 2012-10-08 12:25 ` Lukáš Czerner 0 siblings, 1 reply; 6+ messages in thread From: Eugene Shatokhin @ 2012-10-08 11:09 UTC (permalink / raw) To: linux-ext4; +Cc: Eugene Shatokhin If ext4_mb_init() returns error (e.g. if there is not enough memory), ext4_fill_super() returns 0 rather than the error code. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=48431 The patch fixes that problem. Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru> --- fs/ext4/super.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 982f6fc..7292532 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -3982,6 +3982,7 @@ no_journal: if (err) { ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", err); + ret = err; goto failed_mount5; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix error handling in ext4_fill_super() 2012-10-08 11:09 [PATCH] ext4: fix error handling in ext4_fill_super() Eugene Shatokhin @ 2012-10-08 12:25 ` Lukáš Czerner 2012-10-08 12:32 ` Lukas Czerner 0 siblings, 1 reply; 6+ messages in thread From: Lukáš Czerner @ 2012-10-08 12:25 UTC (permalink / raw) To: Eugene Shatokhin; +Cc: linux-ext4 On Mon, 8 Oct 2012, Eugene Shatokhin wrote: > Date: Mon, 8 Oct 2012 15:09:42 +0400 > From: Eugene Shatokhin <eugene.shatokhin@rosalab.ru> > To: linux-ext4@vger.kernel.org > Cc: Eugene Shatokhin <eugene.shatokhin@rosalab.ru> > Subject: [PATCH] ext4: fix error handling in ext4_fill_super() > > If ext4_mb_init() returns error (e.g. if there is not enough memory), > ext4_fill_super() returns 0 rather than the error code. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=48431 > > The patch fixes that problem. Unfortunately there are other places with this problem as well. I'll send you a different patch to address this problem, let me know what do you think. Thanks! -Lukas > > Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru> > --- > fs/ext4/super.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 982f6fc..7292532 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3982,6 +3982,7 @@ no_journal: > if (err) { > ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", > err); > + ret = err; > goto failed_mount5; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ext4: fix error handling in ext4_fill_super() 2012-10-08 12:25 ` Lukáš Czerner @ 2012-10-08 12:32 ` Lukas Czerner 2012-10-08 13:48 ` Lukáš Czerner 2012-11-08 20:15 ` Theodore Ts'o 0 siblings, 2 replies; 6+ messages in thread From: Lukas Czerner @ 2012-10-08 12:32 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, Lukas Czerner There are some places in ext4_fill_super() where we would not return proper error code if something fails. The confusion is caused probably due to the fact that we have two "kind-of" return variables 'ret'and 'err'. 'ret' is used to return error code from ext4_fill_super() where err is used to store return values from other functions within ext4_fill_super(). However some places were missing the obligatory 'ret = err'. We could put the assignment where it is missing, but we can have better "future proof" solution. Or we could convert the code to use just one, but it would require more rewrites. This commit fixes the problem by returning value from 'err' variable if it is set and 'ret' otherwise in error handling branch of the ext4_fill_super(). The reasoning is that 'ret' value is often set to default "-EINVAL" or explicit value, where 'err' is used to store return value from other functions and should be otherwise zero. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/ext4/super.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 69c55d4..c02c676 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -3223,7 +3223,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) unsigned int i; int needs_recovery, has_huge_files, has_bigalloc; __u64 blocks_count; - int err; + int err = 0; unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO; ext4_group_t first_not_zeroed; @@ -3252,6 +3252,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) for (cp = sb->s_id; (cp = strchr(cp, '/'));) *cp = '!'; + /* -EINVAL is default */ ret = -EINVAL; blocksize = sb_min_blocksize(sb, EXT4_MIN_BLOCK_SIZE); if (!blocksize) { @@ -3629,7 +3630,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) " too large to mount safely on this system"); if (sizeof(sector_t) < 8) ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled"); - ret = err; goto failed_mount; } @@ -3737,7 +3737,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) } if (err) { ext4_msg(sb, KERN_ERR, "insufficient memory"); - ret = err; goto failed_mount3; } @@ -3863,8 +3862,8 @@ no_journal: if (es->s_overhead_clusters) sbi->s_overhead = le32_to_cpu(es->s_overhead_clusters); else { - ret = ext4_calculate_overhead(sb); - if (ret) + err = ext4_calculate_overhead(sb); + if (err) goto failed_mount_wq; } @@ -3876,6 +3875,7 @@ no_journal: alloc_workqueue("ext4-dio-unwritten", WQ_MEM_RECLAIM | WQ_UNBOUND, 1); if (!EXT4_SB(sb)->dio_unwritten_wq) { printk(KERN_ERR "EXT4-fs: failed to create DIO workqueue\n"); + ret = -ENOMEM; goto failed_mount_wq; } @@ -3978,8 +3978,8 @@ no_journal: /* Enable quota usage during mount. */ if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) && !(sb->s_flags & MS_RDONLY)) { - ret = ext4_enable_quotas(sb); - if (ret) + err = ext4_enable_quotas(sb); + if (err) goto failed_mount7; } #endif /* CONFIG_QUOTA */ @@ -4050,7 +4050,7 @@ out_fail: kfree(sbi); out_free_orig: kfree(orig_data); - return ret; + return err ? err : ret; } /* -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix error handling in ext4_fill_super() 2012-10-08 12:32 ` Lukas Czerner @ 2012-10-08 13:48 ` Lukáš Czerner 2012-10-08 14:46 ` Eugene Shatokhin 2012-11-08 20:15 ` Theodore Ts'o 1 sibling, 1 reply; 6+ messages in thread From: Lukáš Czerner @ 2012-10-08 13:48 UTC (permalink / raw) To: Lukas Czerner; +Cc: linux-ext4, tytso, Eugene Shatokhin I forgot to add Eugene to the cc, sorry. -Lukas On Mon, 8 Oct 2012, Lukas Czerner wrote: > Date: Mon, 8 Oct 2012 14:32:54 +0200 > From: Lukas Czerner <lczerner@redhat.com> > To: linux-ext4@vger.kernel.org > Cc: tytso@mit.edu, Lukas Czerner <lczerner@redhat.com> > Subject: [PATCH] ext4: fix error handling in ext4_fill_super() > > There are some places in ext4_fill_super() where we would not return > proper error code if something fails. The confusion is caused probably > due to the fact that we have two "kind-of" return variables 'ret'and > 'err'. > > 'ret' is used to return error code from ext4_fill_super() where err is > used to store return values from other functions within ext4_fill_super(). > However some places were missing the obligatory 'ret = err'. We could > put the assignment where it is missing, but we can have better "future > proof" solution. Or we could convert the code to use just one, but it > would require more rewrites. > > This commit fixes the problem by returning value from 'err' variable if > it is set and 'ret' otherwise in error handling branch of the > ext4_fill_super(). The reasoning is that 'ret' value is often set to > default "-EINVAL" or explicit value, where 'err' is used to store > return value from other functions and should be otherwise zero. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > --- > fs/ext4/super.c | 16 ++++++++-------- > 1 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 69c55d4..c02c676 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3223,7 +3223,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > unsigned int i; > int needs_recovery, has_huge_files, has_bigalloc; > __u64 blocks_count; > - int err; > + int err = 0; > unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO; > ext4_group_t first_not_zeroed; > > @@ -3252,6 +3252,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > for (cp = sb->s_id; (cp = strchr(cp, '/'));) > *cp = '!'; > > + /* -EINVAL is default */ > ret = -EINVAL; > blocksize = sb_min_blocksize(sb, EXT4_MIN_BLOCK_SIZE); > if (!blocksize) { > @@ -3629,7 +3630,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > " too large to mount safely on this system"); > if (sizeof(sector_t) < 8) > ext4_msg(sb, KERN_WARNING, "CONFIG_LBDAF not enabled"); > - ret = err; > goto failed_mount; > } > > @@ -3737,7 +3737,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > } > if (err) { > ext4_msg(sb, KERN_ERR, "insufficient memory"); > - ret = err; > goto failed_mount3; > } > > @@ -3863,8 +3862,8 @@ no_journal: > if (es->s_overhead_clusters) > sbi->s_overhead = le32_to_cpu(es->s_overhead_clusters); > else { > - ret = ext4_calculate_overhead(sb); > - if (ret) > + err = ext4_calculate_overhead(sb); > + if (err) > goto failed_mount_wq; > } > > @@ -3876,6 +3875,7 @@ no_journal: > alloc_workqueue("ext4-dio-unwritten", WQ_MEM_RECLAIM | WQ_UNBOUND, 1); > if (!EXT4_SB(sb)->dio_unwritten_wq) { > printk(KERN_ERR "EXT4-fs: failed to create DIO workqueue\n"); > + ret = -ENOMEM; > goto failed_mount_wq; > } > > @@ -3978,8 +3978,8 @@ no_journal: > /* Enable quota usage during mount. */ > if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) && > !(sb->s_flags & MS_RDONLY)) { > - ret = ext4_enable_quotas(sb); > - if (ret) > + err = ext4_enable_quotas(sb); > + if (err) > goto failed_mount7; > } > #endif /* CONFIG_QUOTA */ > @@ -4050,7 +4050,7 @@ out_fail: > kfree(sbi); > out_free_orig: > kfree(orig_data); > - return ret; > + return err ? err : ret; > } > > /* > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix error handling in ext4_fill_super() 2012-10-08 13:48 ` Lukáš Czerner @ 2012-10-08 14:46 ` Eugene Shatokhin 0 siblings, 0 replies; 6+ messages in thread From: Eugene Shatokhin @ 2012-10-08 14:46 UTC (permalink / raw) To: Lukáš Czerner; +Cc: linux-ext4, tytso On 10/08/2012 05:48 PM, Lukáš Czerner wrote: >> Date: Mon, 8 Oct 2012 14:32:54 +0200 >> From: Lukas Czerner <lczerner@redhat.com> >> To: linux-ext4@vger.kernel.org >> Cc: tytso@mit.edu, Lukas Czerner <lczerner@redhat.com> >> Subject: [PATCH] ext4: fix error handling in ext4_fill_super() The patch looks good to me. Regards, Eugene -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix error handling in ext4_fill_super() 2012-10-08 12:32 ` Lukas Czerner 2012-10-08 13:48 ` Lukáš Czerner @ 2012-11-08 20:15 ` Theodore Ts'o 1 sibling, 0 replies; 6+ messages in thread From: Theodore Ts'o @ 2012-11-08 20:15 UTC (permalink / raw) To: Lukas Czerner; +Cc: linux-ext4 On Mon, Oct 08, 2012 at 02:32:54PM +0200, Lukas Czerner wrote: > There are some places in ext4_fill_super() where we would not return > proper error code if something fails. The confusion is caused probably > due to the fact that we have two "kind-of" return variables 'ret'and > 'err'. > > 'ret' is used to return error code from ext4_fill_super() where err is > used to store return values from other functions within ext4_fill_super(). > However some places were missing the obligatory 'ret = err'. We could > put the assignment where it is missing, but we can have better "future > proof" solution. Or we could convert the code to use just one, but it > would require more rewrites. > > This commit fixes the problem by returning value from 'err' variable if > it is set and 'ret' otherwise in error handling branch of the > ext4_fill_super(). The reasoning is that 'ret' value is often set to > default "-EINVAL" or explicit value, where 'err' is used to store > return value from other functions and should be otherwise zero. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> Applied, thanks. - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-08 22:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-08 11:09 [PATCH] ext4: fix error handling in ext4_fill_super() Eugene Shatokhin 2012-10-08 12:25 ` Lukáš Czerner 2012-10-08 12:32 ` Lukas Czerner 2012-10-08 13:48 ` Lukáš Czerner 2012-10-08 14:46 ` Eugene Shatokhin 2012-11-08 20:15 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).