On Wed, 2019-10-16 at 11:18 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:48AM +0800, Ian Kent wrote:
> > +static const struct fs_parameter_description xfs_fs_parameters = {
> > + .name = "XFS",
> > + .specs = xfs_param_specs,
> > +};
>
> Well spell xfs in lower case in the file system type, so I think we
> should
> spell it the same here.
The problem is that this will probably be used in logging later and
there's a lot of logging that uses the upper case variant.
OTOH if all the log messages were changed to use lower case "xfs" then
one of the problems I see with logging (that name inconsistency) would
go away.
So I'm not sure what I should do here.
>
> Btw, can we keep all the mount code together where most of it already
> is at the top of the file? I know the existing version has some
> remount
> stuff at the bottom, but as that get entirely rewritten we might as
> well
> move it all up.
Yep, sounds good.
>
> > + int silent = fc->sb_flags & SB_SILENT;
>
> The silent variable is only used once, so we might as well remove it.
And again.
>
> > + struct xfs_mount *mp = fc->s_fs_info;
> > +
> > + /*
> > + * mp and ctx are stored in the fs_context when it is
> > + * initialized. mp is transferred to the superblock on
> > + * a successful mount, but if an error occurs before the
> > + * transfer we have to free it here.
> > + */
> > + if (mp) {
> > + xfs_free_names(mp);
> > + kfree(mp);
> > + }
>
> We always pair xfs_free_names with freeing the mount structure.
> I think it would be nice to add an xfs_free_mount that does both
> as a refactoring at the beginning of the series.
Ditto.
>
> > +static const struct fs_context_operations xfs_context_ops = {
> > + .parse_param = xfs_parse_param,
> > + .get_tree = xfs_get_tree,
> > + .reconfigure = xfs_reconfigure,
> > + .free = xfs_fc_free,
> > +};
>
> Should these all get a prefix like xfs_fc_free? Maybe xfs_fsctx
> to be a little bit more descriptive?
Good point, since it's struct fs_context* I think an "xfs_fc_"
prefix on the context related structures and variables would make
the most sense.
I'll do that too.
Ian
