Re: [PATCH V2] nilfs2: convert to use the new mount API

[Eric Sandeen <sandeen@redhat.com>]

On 4/5/24 5:33 AM, Ryusuke Konishi wrote:
> On Fri, Apr 5, 2024 at 12:00 PM Eric Sandeen wrote:
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: Fix call to nilfs2_reconfigure() in nilfs_mount() to ensure
>> fc->root is set.
>>
>> Clean up some extraneous comments and whitespace
>>
>> This one passes your current test script.
>>
>> Thanks,
>> -Eric
> 
> Yeah, this v2 patch resolved the panic issue.  It passed the test
> script in multiple environments, as well as my manual checks for mount
> options with and without using the constant table.  Seems to be
> working perfectly so far.

great!

> I'll get started on the full review, but I'd like to provide feedback
> on style warnings detected by the checkpatch script at this point.
 
> ---
> WARNING: Missing commit description - Add an appropriate one

Not a lot to say; I can note 'interesting' things about the conversion
if that'd be helpful.

> WARNING: function definition argument 'struct super_block *' should
> also have an identifier name
> #146: FILE: fs/nilfs2/nilfs.h:338:
> +extern int nilfs_store_magic(struct super_block *,

As you say this is pre-existing; I can change it if you like, I was just
following current style.

> WARNING: function definition argument 'struct nilfs_super_block *'
> should also have an identifier name
> #146: FILE: fs/nilfs2/nilfs.h:338:
> +extern int nilfs_store_magic(struct super_block *,

same
 
> WARNING: space prohibited between function name and open parenthesis '('
> #220: FILE: fs/nilfs2/super.c:721:
> + fsparam_enum ("errors", Opt_err, nilfs_param_err),

This seems to be the pattern for every filesystem using these calls ...
 
> WARNING: space prohibited between function name and open parenthesis '('
> #221: FILE: fs/nilfs2/super.c:722:
> + fsparam_flag_no ("barrier", Opt_barrier),
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #223: FILE: fs/nilfs2/super.c:724:
> + fsparam_string ("order", Opt_order),
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #224: FILE: fs/nilfs2/super.c:725:
> + fsparam_flag ("norecovery", Opt_norecovery),
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #225: FILE: fs/nilfs2/super.c:726:
> + fsparam_flag_no ("discard", Opt_discard),
> 
> WARNING: Missing a blank line after declarations
> #317: FILE: fs/nilfs2/super.c:770:
> + struct super_block *sb = fc->root->d_sb;
> + nilfs_err(sb,

easy enough to fix.

> WARNING: quoted string split across lines
> #576: FILE: fs/nilfs2/super.c:1193:
> + nilfs_err(s, "invalid option \"cn=%llu\", "
> + "read-only option is not specified",

Just let me know your preference on long strings like this (out-dent?
go past col 80? leave it alone?)

I'll wait for more review before I send a minor update just for these.
(or, if you prefer, feel free to tweak small things on your end.)

thanks,
-Eric

> total: 0 errors, 10 warnings, 563 lines checked
> ---
> 
> Of these, the warning for the function declaration of
> nilfs_store_magic() is an existing issue, so it can be left as is.
> 
> Also, I feel like the warnings for fsparam_{enum,flag,flag_no,string}
> can be ignored for the sake of appearance.  (I will not omit them here
> so as not to make any preconceptions).
> 
> Thanks,
> Ryusuke Konishi
>