答复: [PATCH] ext4: fix error code in ext4_commit_super

Is there any problem with this patch? I did not see a reply, please let me
know if there is a problem. Thanks

-----邮件原件-----
发件人: Fengnan Chang <changfengnan@vivo.com> 
发送时间: 2021年3月29日 11:58
收件人: tytso@mit.edu; adilger.kernel@dilger.ca; linux-ext4@vger.kernel.org
抄送: Fengnan Chang <changfengnan@vivo.com>
主题: [PATCH] ext4: fix error code in ext4_commit_super

We should set the error code when ext4_commit_super check argument failed.

Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/ext4/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 	 * If the file system is mounted read-only, don't update the
--
2.29.0

On Apr 1, 2021, at 9:32 PM, <changfengnan@vivo.com> <changfengnan@vivo.com> wrote:
> 
> Is there any problem with this patch? I did not see a reply, please let me
> know if there is a problem. Thanks

It's only been a few days since the patch was first posted, so nobody has
had a chance to review it yet.  The patch looks "obviously correct" at
first glance, but often changing code is not as obvious as first expected.
Also, is "-EINVAL" the best code here?  For "block_device_ejected()" it
might be more clear to return "-ENODEV" so that it prints "No such device"
instead of "Invalid argument" in userspace.

> 发件人: Fengnan Chang <changfengnan@vivo.com>
> 发送时间: 2021年3月29日 11:58
> 收件人: tytso@mit.edu; adilger.kernel@dilger.ca; linux-ext4@vger.kernel.org
> 
> We should set the error code when ext4_commit_super check argument failed.

It would be useful if this also described how this problem was hit, or
what caused this issue to be seen/fixed.  "Found while reviewing code"
is OK, if that is the case, or "crashed during mount when ejecting a
floppy disk", or whatever.  That makes it more clear how important the
bug fix is to be landed and backported.

It would also be useful to include a "Fixes:" label to show which patch
caused the problem, and help decide which stable kernels need this patch.
From running "git blame fs/ext4/super.c" appears that commit 2d01ddc86606
("ext4: save error info to sb through journal if available") introduced
the problem, but it seems like that patch only copied it from older code.
However, further digging shows commit c4be0c1dc4cdc ("filesystem freeze:
add error handling of write_super_lockfs/unlockfs") added this particular
code (error = 0; return error).  Before that time, no error was returned
from this function at all, so the commit message should include:

Fixes: c4be0c1dc4cdc ("filesystem freeze: add error handling of write_super_lockfs/unlockfs")

Cheers, Andreas

> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c index
> 03373471131c..5440b8ff86a8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5501,7 +5501,7 @@ static int ext4_commit_super(struct super_block *sb,
> int sync)
> 	int error = 0;
> 
> 	if (!sbh || block_device_ejected(sb))
> -		return error;
> +		return -EINVAL;
> 
> 	/*
> 	 * If the file system is mounted read-only, don't update the
> --
> 2.29.0
> 
> 
> 


Cheers, Andreas

Thanks for your advice, We did found this problem during code review.
I will revise it according to your opinion.

-----邮件原件-----
发件人: Andreas Dilger <adilger@dilger.ca>
发送时间: 2021年4月2日 15:58
收件人: changfengnan@vivo.com
抄送: Theodore Y. Ts'o <tytso@mit.edu>; Ext4 Developers List 
<linux-ext4@vger.kernel.org>
主题: Re: [PATCH] ext4: fix error code in ext4_commit_super

On Apr 1, 2021, at 9:32 PM, <changfengnan@vivo.com> <changfengnan@vivo.com> 
wrote:
>
> Is there any problem with this patch? I did not see a reply, please
> let me know if there is a problem. Thanks

It's only been a few days since the patch was first posted, so nobody has 
had a chance to review it yet.  The patch looks "obviously correct" at first 
glance, but often changing code is not as obvious as first expected.
Also, is "-EINVAL" the best code here?  For "block_device_ejected()" it 
might be more clear to return "-ENODEV" so that it prints "No such device"
instead of "Invalid argument" in userspace.

> 发件人: Fengnan Chang <changfengnan@vivo.com>
> 发送时间: 2021年3月29日 11:58
> 收件人: tytso@mit.edu; adilger.kernel@dilger.ca;
> linux-ext4@vger.kernel.org
>
> We should set the error code when ext4_commit_super check argument failed.

It would be useful if this also described how this problem was hit, or what 
caused this issue to be seen/fixed.  "Found while reviewing code"
is OK, if that is the case, or "crashed during mount when ejecting a floppy 
disk", or whatever.  That makes it more clear how important the bug fix is 
to be landed and backported.

It would also be useful to include a "Fixes:" label to show which patch 
caused the problem, and help decide which stable kernels need this patch.
From running "git blame fs/ext4/super.c" appears that commit 2d01ddc86606
("ext4: save error info to sb through journal if available") introduced the 
problem, but it seems like that patch only copied it from older code.
However, further digging shows commit c4be0c1dc4cdc ("filesystem freeze:
add error handling of write_super_lockfs/unlockfs") added this particular 
code (error = 0; return error).  Before that time, no error was returned 
from this function at all, so the commit message should include:

Fixes: c4be0c1dc4cdc ("filesystem freeze: add error handling of 
write_super_lockfs/unlockfs")

Cheers, Andreas

> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c index
> 03373471131c..5440b8ff86a8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5501,7 +5501,7 @@ static int ext4_commit_super(struct super_block
> *sb, int sync)
> 	int error = 0;
>
> 	if (!sbh || block_device_ejected(sb))
> -		return error;
> +		return -EINVAL;
>
> 	/*
> 	 * If the file system is mounted read-only, don't update the
> --
> 2.29.0
>
>
>


Cheers, Andreas