[PATCH] erofs: add error log in erofs_fc_parse_param

[PATCH] erofs: add error log in erofs_fc_parse_param

[Chen Linxuan]

While reading erofs code, I notice that `erofs_fc_parse_param` will
return -ENOPARAM, which means that erofs do not support this option,
without report anything when `fs_parse` return an unknown `opt`.

But if an option is unknown to erofs, I mean that option not in
`erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
which means that `erofs_fs_parameters` should has returned earlier.

Entering `default` means `fs_parse` return something we unexpected.
I am not sure about it but I think we should return -EINVAL here,
just like `xfs_fs_parse_param`.

Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
---
 fs/erofs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 1fc5623c3a4d..67fc4c1deb98 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -509,7 +509,8 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 #endif
 		break;
 	default:
-		return -ENOPARAM;
+		errorfc(fc, "%s option not supported", param->key);
+		return -EINVAL;
 	}
 	return 0;
 }
-- 
2.43.0

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Gao Xiang]

Hi Linxuan,

On 2025/1/17 16:52, Chen Linxuan wrote:
> While reading erofs code, I notice that `erofs_fc_parse_param` will
> return -ENOPARAM, which means that erofs do not support this option,
> without report anything when `fs_parse` return an unknown `opt`.
> 
> But if an option is unknown to erofs, I mean that option not in
> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
> which means that `erofs_fs_parameters` should has returned earlier.
> 
> Entering `default` means `fs_parse` return something we unexpected.
> I am not sure about it but I think we should return -EINVAL here,
> just like `xfs_fs_parse_param`.
> 
> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>

I think the default branch is actually deadcode here, see
erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM

then vfs_parse_fs_param() will show "Unknown parameter".

Maybe we could just kill `default:` branch...

Thanks,
Gao Xiang


> ---
>   fs/erofs/super.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 1fc5623c3a4d..67fc4c1deb98 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -509,7 +509,8 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>   #endif
>   		break;
>   	default:
> -		return -ENOPARAM;
> +		errorfc(fc, "%s option not supported", param->key);
> +		return -EINVAL;
>   	}
>   	return 0;
>   }

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Chen Linxuan]

On Fri, 2025-01-17 at 17:28 +0800, Gao Xiang wrote:
> Hi Linxuan,
> 
> On 2025/1/17 16:52, Chen Linxuan wrote:
> > While reading erofs code, I notice that `erofs_fc_parse_param` will
> > return -ENOPARAM, which means that erofs do not support this option,
> > without report anything when `fs_parse` return an unknown `opt`.
> > 
> > But if an option is unknown to erofs, I mean that option not in
> > `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
> > which means that `erofs_fs_parameters` should has returned earlier.
> > 
> > Entering `default` means `fs_parse` return something we unexpected.
> > I am not sure about it but I think we should return -EINVAL here,
> > just like `xfs_fs_parse_param`.
> > 
> > Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> 
> I think the default branch is actually deadcode here, see
> erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM
> 
> then vfs_parse_fs_param() will show "Unknown parameter".
> 
> Maybe we could just kill `default:` branch...

ext4 do not have a `default:` branch, but xfs return -EINVAL.

I think `default:` branch can report error when `fs_parse` or
`erofs_fs_parameters` goes wrong.

But I am OK to kill `default:` branch.

> 
> Thanks,
> Gao Xiang
> 
> 
> > ---
> >   fs/erofs/super.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > index 1fc5623c3a4d..67fc4c1deb98 100644
> > --- a/fs/erofs/super.c
> > +++ b/fs/erofs/super.c
> > @@ -509,7 +509,8 @@ static int erofs_fc_parse_param(struct fs_context *fc,
> >   #endif
> >   		break;
> >   	default:
> > -		return -ENOPARAM;
> > +		errorfc(fc, "%s option not supported", param->key);
> > +		return -EINVAL;
> >   	}
> >   	return 0;
> >   }
> 
> 
> 

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Gao Xiang]

On 2025/1/17 17:50, Chen Linxuan wrote:
> On Fri, 2025-01-17 at 17:28 +0800, Gao Xiang wrote:
>> Hi Linxuan,
>>
>> On 2025/1/17 16:52, Chen Linxuan wrote:
>>> While reading erofs code, I notice that `erofs_fc_parse_param` will
>>> return -ENOPARAM, which means that erofs do not support this option,
>>> without report anything when `fs_parse` return an unknown `opt`.
>>>
>>> But if an option is unknown to erofs, I mean that option not in
>>> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
>>> which means that `erofs_fs_parameters` should has returned earlier.
>>>
>>> Entering `default` means `fs_parse` return something we unexpected.
>>> I am not sure about it but I think we should return -EINVAL here,
>>> just like `xfs_fs_parse_param`.
>>>
>>> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
>>
>> I think the default branch is actually deadcode here, see
>> erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM
>>
>> then vfs_parse_fs_param() will show "Unknown parameter".
>>
>> Maybe we could just kill `default:` branch...
> 
> ext4 do not have a `default:` branch, but xfs return -EINVAL.
> 
> I think `default:` branch can report error when `fs_parse` or
> `erofs_fs_parameters` goes wrong.

How can it go wrong?

Thanks,
Gao Xiang

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Chen Linxuan]

On Fri, 2025-01-17 at 17:54 +0800, Gao Xiang wrote:
> 
> On 2025/1/17 17:50, Chen Linxuan wrote:
> > On Fri, 2025-01-17 at 17:28 +0800, Gao Xiang wrote:
> > > Hi Linxuan,
> > > 
> > > On 2025/1/17 16:52, Chen Linxuan wrote:
> > > > While reading erofs code, I notice that `erofs_fc_parse_param` will
> > > > return -ENOPARAM, which means that erofs do not support this option,
> > > > without report anything when `fs_parse` return an unknown `opt`.
> > > > 
> > > > But if an option is unknown to erofs, I mean that option not in
> > > > `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
> > > > which means that `erofs_fs_parameters` should has returned earlier.
> > > > 
> > > > Entering `default` means `fs_parse` return something we unexpected.
> > > > I am not sure about it but I think we should return -EINVAL here,
> > > > just like `xfs_fs_parse_param`.
> > > > 
> > > > Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> > > 
> > > I think the default branch is actually deadcode here, see
> > > erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM
> > > 
> > > then vfs_parse_fs_param() will show "Unknown parameter".
> > > 
> > > Maybe we could just kill `default:` branch...
> > 
> > ext4 do not have a `default:` branch, but xfs return -EINVAL.
> > 
> > I think `default:` branch can report error when `fs_parse` or
> > `erofs_fs_parameters` goes wrong.
> 
> How can it go wrong?

What if we forget to update the switch branch for a new option?

> 
> Thanks,
> Gao Xiang
> 
> 

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Chen Linxuan]

On Fri, 2025-01-17 at 17:54 +0800, Gao Xiang wrote:
> 
> On 2025/1/17 17:50, Chen Linxuan wrote:
> > On Fri, 2025-01-17 at 17:28 +0800, Gao Xiang wrote:
> > > Hi Linxuan,
> > > 
> > > On 2025/1/17 16:52, Chen Linxuan wrote:
> > > > While reading erofs code, I notice that `erofs_fc_parse_param` will
> > > > return -ENOPARAM, which means that erofs do not support this option,
> > > > without report anything when `fs_parse` return an unknown `opt`.
> > > > 
> > > > But if an option is unknown to erofs, I mean that option not in
> > > > `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
> > > > which means that `erofs_fs_parameters` should has returned earlier.
> > > > 
> > > > Entering `default` means `fs_parse` return something we unexpected.
> > > > I am not sure about it but I think we should return -EINVAL here,
> > > > just like `xfs_fs_parse_param`.
> > > > 
> > > > Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> > > 
> > > I think the default branch is actually deadcode here, see
> > > erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM
> > > 
> > > then vfs_parse_fs_param() will show "Unknown parameter".
> > > 
> > > Maybe we could just kill `default:` branch...
> > 
> > ext4 do not have a `default:` branch, but xfs return -EINVAL.
> > 
> > I think `default:` branch can report error when `fs_parse` or
> > `erofs_fs_parameters` goes wrong.
> 
> How can it go wrong?

I will push a new patch just remove the `default:` branch later.

> 
> Thanks,
> Gao Xiang
> 
> 

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Gao Xiang]

On 2025/1/17 18:00, Chen Linxuan wrote:
> On Fri, 2025-01-17 at 17:54 +0800, Gao Xiang wrote:
>>
>> On 2025/1/17 17:50, Chen Linxuan wrote:
>>> On Fri, 2025-01-17 at 17:28 +0800, Gao Xiang wrote:
>>>> Hi Linxuan,
>>>>
>>>> On 2025/1/17 16:52, Chen Linxuan wrote:
>>>>> While reading erofs code, I notice that `erofs_fc_parse_param` will
>>>>> return -ENOPARAM, which means that erofs do not support this option,
>>>>> without report anything when `fs_parse` return an unknown `opt`.
>>>>>
>>>>> But if an option is unknown to erofs, I mean that option not in
>>>>> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
>>>>> which means that `erofs_fs_parameters` should has returned earlier.
>>>>>
>>>>> Entering `default` means `fs_parse` return something we unexpected.
>>>>> I am not sure about it but I think we should return -EINVAL here,
>>>>> just like `xfs_fs_parse_param`.
>>>>>
>>>>> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
>>>>
>>>> I think the default branch is actually deadcode here, see
>>>> erofs_fc_parse_param() -> fs_parse() -> fs_lookup_key() -> -ENOPARAM
>>>>
>>>> then vfs_parse_fs_param() will show "Unknown parameter".
>>>>
>>>> Maybe we could just kill `default:` branch...
>>>
>>> ext4 do not have a `default:` branch, but xfs return -EINVAL.
>>>
>>> I think `default:` branch can report error when `fs_parse` or
>>> `erofs_fs_parameters` goes wrong.
>>
>> How can it go wrong?
> 
> What if we forget to update the switch branch for a new option?

Then it's clearly a bug (we don't even handle the new option),
I think we shouldn't consider it as a normal case.

Thanks,
Gao Xiang

> 
>>
>> Thanks,
>> Gao Xiang
>>
>>

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Ian Kent]

On 17/1/25 16:52, Chen Linxuan wrote:
> While reading erofs code, I notice that `erofs_fc_parse_param` will
> return -ENOPARAM, which means that erofs do not support this option,
> without report anything when `fs_parse` return an unknown `opt`.
>
> But if an option is unknown to erofs, I mean that option not in
> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
> which means that `erofs_fs_parameters` should has returned earlier.

I'm pretty sure than the vfs deals with reporting unknown options

and returns -EINVAL already.


I think the caller oferofs_fc_parse_param() is vfs_parse_fs_param()

and for an -ENOPARAM return will ultimately do this:

return invalf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, 
param->key);

which does this.

The thing about this is the mount API macro deals with (or it should,

although I'm not sure that's completely sorted out yet) logging the

message to the console log as well as possibly making it available to

mount api system calls. I'm pretty sure this change will prevent the

error message being available for mount api system calls to retrieve.

Ian

>
> Entering `default` means `fs_parse` return something we unexpected.
> I am not sure about it but I think we should return -EINVAL here,
> just like `xfs_fs_parse_param`.
>
> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> ---
>   fs/erofs/super.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 1fc5623c3a4d..67fc4c1deb98 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -509,7 +509,8 @@ static int erofs_fc_parse_param(struct fs_context *fc,
>   #endif
>   		break;
>   	default:
> -		return -ENOPARAM;
> +		errorfc(fc, "%s option not supported", param->key);
> +		return -EINVAL;
>   	}
>   	return 0;
>   }

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Ian Kent]

On 18/1/25 08:45, Ian Kent wrote:
> On 17/1/25 16:52, Chen Linxuan wrote:
>> While reading erofs code, I notice that `erofs_fc_parse_param` will
>> return -ENOPARAM, which means that erofs do not support this option,
>> without report anything when `fs_parse` return an unknown `opt`.
>>
>> But if an option is unknown to erofs, I mean that option not in
>> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
>> which means that `erofs_fs_parameters` should has returned earlier.
>
> I'm pretty sure than the vfs deals with reporting unknown options
>
> and returns -EINVAL already.
>
>
> I think the caller oferofs_fc_parse_param() is vfs_parse_fs_param()
>
> and for an -ENOPARAM return will ultimately do this:
>
> return invalf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, 
> param->key);
>
> which does this.
>
> The thing about this is the mount API macro deals with (or it should,
>
> although I'm not sure that's completely sorted out yet) logging the
>
> message to the console log as well as possibly making it available to
>
> mount api system calls. I'm pretty sure this change will prevent the
>
> error message being available for mount api system calls to retrieve.

Actually no, removing the default switch branch does look like the right 
thing

to do, my mistake. The call to fs_parse() will return -NOPARAM so the

switch doesn't need to handle that case.


Oops!

>
> Ian
>
>>
>> Entering `default` means `fs_parse` return something we unexpected.
>> I am not sure about it but I think we should return -EINVAL here,
>> just like `xfs_fs_parse_param`.
>>
>> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
>> ---
>>   fs/erofs/super.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index 1fc5623c3a4d..67fc4c1deb98 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -509,7 +509,8 @@ static int erofs_fc_parse_param(struct fs_context 
>> *fc,
>>   #endif
>>           break;
>>       default:
>> -        return -ENOPARAM;
>> +        errorfc(fc, "%s option not supported", param->key);
>> +        return -EINVAL;
>>       }
>>       return 0;
>>   }

Re: [PATCH] erofs: add error log in erofs_fc_parse_param

[Gao Xiang]

On 2025/1/18 09:25, Ian Kent wrote:
> On 18/1/25 08:45, Ian Kent wrote:
>> On 17/1/25 16:52, Chen Linxuan wrote:
>>> While reading erofs code, I notice that `erofs_fc_parse_param` will
>>> return -ENOPARAM, which means that erofs do not support this option,
>>> without report anything when `fs_parse` return an unknown `opt`.
>>>
>>> But if an option is unknown to erofs, I mean that option not in
>>> `erofs_fs_parameters` at all, `fs_parse` will return -ENOPARAM,
>>> which means that `erofs_fs_parameters` should has returned earlier.
>>
>> I'm pretty sure than the vfs deals with reporting unknown options
>>
>> and returns -EINVAL already.
>>
>>
>> I think the caller oferofs_fc_parse_param() is vfs_parse_fs_param()
>>
>> and for an -ENOPARAM return will ultimately do this:
>>
>> return invalf(fc, "%s: Unknown parameter '%s'", fc->fs_type->name, param->key);
>>
>> which does this.
>>
>> The thing about this is the mount API macro deals with (or it should,
>>
>> although I'm not sure that's completely sorted out yet) logging the
>>
>> message to the console log as well as possibly making it available to
>>
>> mount api system calls. I'm pretty sure this change will prevent the
>>
>> error message being available for mount api system calls to retrieve.
> 
> Actually no, removing the default switch branch does look like the right thing
> 
> to do, my mistake. The call to fs_parse() will return -NOPARAM so the
> 
> switch doesn't need to handle that case.
> 
> 
> Oops!

Yeah, actually dead code..

Thanks,
Gao Xiang

> 
>>
>> Ian
>>
>>>