In this email, I will reply regarding style issues.
On Sat, Apr 6, 2024 at 12:04 AM Eric Sandeen wrote:
>
> 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.
>
It's OK if you include a commit log with the patch that you think is
the final version.
I also don't think a long explanation is necessary for this, but I'll
leave it to you.
> > 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
When modifying declarations, I take the opportunity to add missing
identifier names to their arguments and delete "extern" specifiers to
eliminate checkpatch warnings, so I would appreciate it if you could
correct it as shown below.
int nilfs_store_magic(struct super_block *sb, struct nilfs_super_block *sbp);
>
> > 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 ...
Yes, I think it looks better now, please ignore these warnings.
>
> > 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?)
Nowadays, when adding a new message or modifying an existing message,
I prioritize its searchability and allow past col 80 only for message
strings.
(I used to split messages to adhere to the 80 characters constraint. )
So, if you don't mind, please fix this checkpatch warning.
>
> 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
> >
>
