Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 16 Aug 2021 16:14:17 +0300
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Christoph Hellwig <hch@lst.de>
Cc:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>,
        ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting
Message-ID: <20210816131417.4mix6s2nzuxhkh53@kari-VirtualBox>
References: <20210816024703.107251-1-kari.argillander@gmail.com>
 <20210816024703.107251-2-kari.argillander@gmail.com>
 <20210816123619.GB17355@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210816123619.GB17355@lst.de>
Precedence: bulk

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/