Thank you for taking time to review. I really appreciated it.
On Mon, Aug 16, 2021 at 02:36:19PM +0200, Christoph Hellwig wrote:
> > +/*
> > + * ntfs_load_nls
> > + *
>
> No need to state the function name here.
This is current way of doing this in fs/ntfs3. I just like that things
are same kind in one driver. I agree that this may not be good way.
> > + * Load nls table or if @nls is utf8 then return NULL because
> > + * nls=utf8 is totally broken.
> > + */
> > +static struct nls_table *ntfs_load_nls(char *nls)
> > +{
> > + struct nls_table *ret;
> > +
> > + if (!nls)
> > + return ERR_PTR(-EINVAL);
> > + if (strcmp(nls, "utf8"))
> > + return NULL;
> > + if (strcmp(nls, CONFIG_NLS_DEFAULT))
> > + return load_nls_default();
> > +
> > + ret = load_nls(nls);
> > + if (!ret)
> > + return ERR_PTR(-EINVAL);
> > +
> > + return ret;
> > +}
>
> This looks like something quite generic and not file system specific.
> But I haven't found time to look at the series from Pali how this all
> fits together.
It is quite generic I agree. Pali's series not implemeted any new way
doing this thing. In many cases Pali uses just load_nls and not
load_nls_default. This function basically use that if possible. It seems
that load_nls_default does not need error path so that's why it is nicer
to use.
One though is to implement api function load_nls_or_utf8(). Then we do not
need to test this utf8 stuff in all places.
> > +// clang-format off
>
> Please don't use C++ comments. And we also should not put weird
> formatter annotations into the kernel source anyway.
This is just a way ntfs3 do this but I agree totally and will take this
off. I did not even like it myself.
> > +static void ntfs_default_options(struct ntfs_mount_options *opts)
> > {
> > opts->fs_uid = current_uid();
> > opts->fs_gid = current_gid();
> > + opts->fs_fmask_inv = ~current_umask();
> > + opts->fs_dmask_inv = ~current_umask();
> > + opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> > +}
>
> This function seems pretty pointless with a single trivial caller.
Yeah it is just because then no comment needed and other reason was that
I can but this closer to ntfs_fs_parse_param() so that when reading code
all parameter code is one place.
> > +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>
> Please avoid the overly long line.
Thanks will fix.
>
> > + break;
> > + case Opt_showmeta:
> > + opts->showmeta = result.negated ? 0 : 1;
> > + break;
> > + case Opt_nls:
> > + unload_nls(opts->nls);
> > +
> > + opts->nls = ntfs_load_nls(param->string);
> > + if (IS_ERR(opts->nls)) {
> > + return invalf(fc, "ntfs3: Cannot load nls %s",
> > + param->string);
> > }
>
> So instead of unloading here, why not set keep a copy of the string
> in the mount options structure and only load the actual table after
> option parsing has finished?
I did actually do this first but then I test this way and code get lot
cleaner. But I can totally change it back to "string loading".
>
> > + struct ntfs_mount_options *new_opts = fc->s_fs_info;
>
> Does this rely on the mount_options being the first member in struct
> ntfs_sb_info? If so that is a landmine for future changes.
>
> > +/*
> > + * Set up the filesystem mount context.
> > + */
> > +static int ntfs_init_fs_context(struct fs_context *fc)
> > +{
> > + struct ntfs_sb_info *sbi;
> > +
> > + sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
>
> Not related to your patch, but why does ntfs3 have kmalloc wrappers
> like this?
I do not know. I actually also suggested changing this (link). This might
even confuse some static analyzer tools.
https://lore.kernel.org/linux-fsdevel/20210103231755.bcmyalz3maq4ama2@kari-VirtualBox/
