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@xxxxxxxxxx>
>> ---
>>
>> 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
>
