by Kari Argillander

[permalink] [raw]

Subject: Re: [PATCH v4 7/9] fs/ntfs3: Add iocharset= mount option as alias for nls=

On Mon, Oct 11, 2021 at 08:14:28PM +0300, Konstantin Komarov wrote:
>
>
> On 09.10.2021 17:33, Kari Argillander wrote:
> > Choose to add Linus to CC so that he also knows whats coming.
> >
> > On Sat, Oct 09, 2021 at 01:42:52PM +0200, Pali Roh?r wrote:
> >> Hello!
> >>
> >> This patch have not been applied yet:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ntfs3/super.c#n247
> >>
> >> What happened that in upstream tree is still only nls= option and not
> >> this iocharset=?
> >
> > Very valid question. For some reason Konstantin has not sended pr to
> > Linus. I have also address my concern that pr is not yet sended and he
> > will make very massive "patch dumb" to rc6/rc7. See thread [1]. There is
> > about 50-70 patch already which he will send to rc6/rc7. I have get also
> > impression that patches which are not yet even applied to ntfs3 tree [2]
> > will be also sended to rc6/rc7. There is lot of refactoring and new
> > algorithms which imo are not rc material. I have sended many message to
> > Konstantin about this topic, but basically ignored.
> >
> > Basically we do not have anything for next merge window and every patch
> > will be sended for 5.15.
> >
> > [1] lore.kernel.org/lkml/20210925082823.fo2wm62xlcexhwvi@kari-VirtualBox
> > [2] https://github.com/Paragon-Software-Group/linux-ntfs3/commits/master
> >
> > Argillander
> >
>
> Hello.
>
> I was planning to send pull request on Friday 08.10.
> But there is still one panic, that wasn't resolved [1].
> It seems to be tricky, so I'll be content even with quick band-aid [2].
> After confirming, that it works, I plan on sending pull request.
> I don't want for this panic to remain in 5.15.

Of course we won't want panic to remain 5.15, but that does not mean you
can't send pr for other patches. You can still send other pr later for
other rc's. If you will send pr late it might be that Linus will not
apply any of them because it will be too big pr for late rc. I'm sure he
will be resonable in first time because ntfs3 is new driver and will
accept pr in this case, but this cannot come habbit.

Normally rc cycle is for this kind of things. We fix things we broke
during merge window, but right now it is real danger that we introduce
many bugs because people cannot test ntfs3 because you hold on patches.
They belong to also to Linus tree not just to linux-next. Many user use
Linus tree and there are many who send good bug reports.

But do not take this too harsh. I'm just personally worried here. Also I
think you as maintainer had improved well in short time.

> [1]: https://lore.kernel.org/ntfs3/[email protected]/
> [2]: https://lore.kernel.org/ntfs3/[email protected]/
>
> >>
> >> On Wednesday 08 September 2021 21:09:38 Pali Roh?r wrote:
> >>> On Tuesday 07 September 2021 18:35:55 Kari Argillander wrote:
> >>>> Other fs drivers are using iocharset= mount option for specifying charset.
> >>>> So add it also for ntfs3 and mark old nls= mount option as deprecated.
> >>>>
> >>>> Signed-off-by: Kari Argillander <[email protected]>
> >>>
> >>> Reviewed-by: Pali Roh?r <[email protected]>
> >>>
> >>>> ---
> >>>> Documentation/filesystems/ntfs3.rst | 4 ++--
> >>>> fs/ntfs3/super.c | 18 +++++++++++-------
> >>>> 2 files changed, 13 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/filesystems/ntfs3.rst b/Documentation/filesystems/ntfs3.rst
> >>>> index af7158de6fde..ded706474825 100644
> >>>> --- a/Documentation/filesystems/ntfs3.rst
> >>>> +++ b/Documentation/filesystems/ntfs3.rst
> >>>> @@ -32,12 +32,12 @@ generic ones.
> >>>>
> >>>> ===============================================================================
> >>>>
> >>>> -nls=name This option informs the driver how to interpret path
> >>>> +iocharset=name This option informs the driver how to interpret path
> >>>> strings and translate them to Unicode and back. If
> >>>> this option is not set, the default codepage will be
> >>>> used (CONFIG_NLS_DEFAULT).
> >>>> Examples:
> >>>> - 'nls=utf8'
> >>>> + 'iocharset=utf8'
> >>>>
> >>>> uid=
> >>>> gid=
> >>>> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> >>>> index 729ead6f2fac..503e2e23f711 100644
> >>>> --- a/fs/ntfs3/super.c
> >>>> +++ b/fs/ntfs3/super.c
> >>>> @@ -226,7 +226,7 @@ enum Opt {
> >>>> Opt_nohidden,
> >>>> Opt_showmeta,
> >>>> Opt_acl,
> >>>> - Opt_nls,
> >>>> + Opt_iocharset,
> >>>> Opt_prealloc,
> >>>> Opt_no_acs_rules,
> >>>> Opt_err,
> >>>> @@ -245,9 +245,13 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
> >>>> fsparam_flag_no("hidden", Opt_nohidden),
> >>>> fsparam_flag_no("acl", Opt_acl),
> >>>> fsparam_flag_no("showmeta", Opt_showmeta),
> >>>> - fsparam_string("nls", Opt_nls),
> >>>> fsparam_flag_no("prealloc", Opt_prealloc),
> >>>> fsparam_flag("no_acs_rules", Opt_no_acs_rules),
> >>>> + fsparam_string("iocharset", Opt_iocharset),
> >>>> +
> >>>> + __fsparam(fs_param_is_string,
> >>>> + "nls", Opt_iocharset,
> >>>> + fs_param_deprecated, NULL),
> >>>> {}
> >>>> };
> >>>>
> >>>> @@ -346,7 +350,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
> >>>> case Opt_showmeta:
> >>>> opts->showmeta = result.negated ? 0 : 1;
> >>>> break;
> >>>> - case Opt_nls:
> >>>> + case Opt_iocharset:
> >>>> kfree(opts->nls_name);
> >>>> opts->nls_name = param->string;
> >>>> param->string = NULL;
> >>>> @@ -380,11 +384,11 @@ static int ntfs_fs_reconfigure(struct fs_context *fc)
> >>>> new_opts->nls = ntfs_load_nls(new_opts->nls_name);
> >>>> if (IS_ERR(new_opts->nls)) {
> >>>> new_opts->nls = NULL;
> >>>> - errorf(fc, "ntfs3: Cannot load nls %s", new_opts->nls_name);
> >>>> + errorf(fc, "ntfs3: Cannot load iocharset %s", new_opts->nls_name);
> >>>> return -EINVAL;
> >>>> }
> >>>> if (new_opts->nls != sbi->options->nls)
> >>>> - return invalf(fc, "ntfs3: Cannot use different nls when remounting!");
> >>>> + return invalf(fc, "ntfs3: Cannot use different iocharset when remounting!");
> >>>>
> >>>> sync_filesystem(sb);
> >>>>
> >>>> @@ -528,9 +532,9 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
> >>>> if (opts->dmask)
> >>>> seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
> >>>> if (opts->nls)
> >>>> - seq_printf(m, ",nls=%s", opts->nls->charset);
> >>>> + seq_printf(m, ",iocharset=%s", opts->nls->charset);
> >>>> else
> >>>> - seq_puts(m, ",nls=utf8");
> >>>> + seq_puts(m, ",iocharset=utf8");
> >>>> if (opts->sys_immutable)
> >>>> seq_puts(m, ",sys_immutable");
> >>>> if (opts->discard)
> >>>> --
> >>>> 2.25.1
> >>>>