diff mbox series

[v5,05/17] xfs: mount-api - refactor suffix_kstrtoint()

Message ID 157062063684.32346.12253005903079702405.stgit@fedora-28
State New, archived
Headers show
Series xfs: mount API patch series | expand

Commit Message

Ian Kent Oct. 9, 2019, 11:30 a.m. UTC
The mount-api doesn't have a "human unit" parse type yet so
the options that have values like "10k" etc. still need to
be converted by the fs.

But the value comes to the fs as a string (not a substring_t
type) so there's a need to change the conversion function to
take a character string instead.

After refactoring xfs_parseargs() and changing it to use
xfs_parse_param() match_kstrtoint() will no longer be used
and will be removed.

Signed-off-by: Ian Kent <raven@themaw.net>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_super.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Oct. 9, 2019, 2:48 p.m. UTC | #1
On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote:
> The mount-api doesn't have a "human unit" parse type yet so
> the options that have values like "10k" etc. still need to
> be converted by the fs.

Any reason you don't lift it to the the core mount api code?
Al Viro Oct. 9, 2019, 3:21 p.m. UTC | #2
On Wed, Oct 09, 2019 at 07:48:59AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote:
> > The mount-api doesn't have a "human unit" parse type yet so
> > the options that have values like "10k" etc. still need to
> > be converted by the fs.
> 
> Any reason you don't lift it to the the core mount api code?

The garbage enum fs_parameter_type thing.  Once we kill that
(and I *will* kill it), sure - it's a sufficiently common
case to have helpers for it around.  But until then I'll
veto any further additions to that trash pile.

Look at fs_parse() - every sodding value added to that enum
turns into an extra case in a large switch.  It's unsustainable
in that form and I *really* don't want to be in position of
gatekeeper deciding what is and what isn't sufficiently useful
to go there.

What we need to do is to turn fs_parameter_type into a pointer
to function.  With fs_param_is_bool et.al. becoming instances
of such, and fs_parse() switch from hell turning into
	err = p->type(p, param, result);

That won't affect the existing macros or any filesystem code.
If some filesystem wants to have helpers of its own - more
power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0)
and be done with that.

The thing is, "here's a commonly useful helper, let's take it
to fs/*.c" is much easier than "add a new constant to that
global enum and a case to that ever-growing switch in fs_parser.c".
Christoph Hellwig Oct. 9, 2019, 3:29 p.m. UTC | #3
On Wed, Oct 09, 2019 at 04:21:27PM +0100, Al Viro wrote:
> What we need to do is to turn fs_parameter_type into a pointer
> to function.  With fs_param_is_bool et.al. becoming instances
> of such, and fs_parse() switch from hell turning into
> 	err = p->type(p, param, result);
> 
> That won't affect the existing macros or any filesystem code.
> If some filesystem wants to have helpers of its own - more
> power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0)
> and be done with that.

Actually, while we could keep the old macros around at least
temporarily for existing users I think killing them actually would
improve the file systems as well.

This:

static const struct fs_parameter_spec afs_param_specs[] = {
	{ "autocell",	Opt_autocell,	fs_parse_flag },
	{ "dyn",	Opt_dyn,	fs_parse_flag },
	{ "flock",	Opt_flock,	fs_parse_enum },
	{ "source",	Opt_source,	fs_parse_string },
        {}
};


is a lot more obvious than:

static const struct fs_parameter_spec afs_param_specs[] = {
        fsparam_flag  ("autocell",      Opt_autocell),
        fsparam_flag  ("dyn",           Opt_dyn),
        fsparam_enum  ("flock",         Opt_flock),
        fsparam_string("source",        Opt_source),
        {}
};
Al Viro Oct. 9, 2019, 4:03 p.m. UTC | #4
On Wed, Oct 09, 2019 at 08:29:11AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 04:21:27PM +0100, Al Viro wrote:
> > What we need to do is to turn fs_parameter_type into a pointer
> > to function.  With fs_param_is_bool et.al. becoming instances
> > of such, and fs_parse() switch from hell turning into
> > 	err = p->type(p, param, result);
> > 
> > That won't affect the existing macros or any filesystem code.
> > If some filesystem wants to have helpers of its own - more
> > power to it, just use __fsparam(my_bloody_helper, "foo", Opt_foo, 0)
> > and be done with that.
> 
> Actually, while we could keep the old macros around at least
> temporarily for existing users I think killing them actually would
> improve the file systems as well.
> 
> This:
> 
> static const struct fs_parameter_spec afs_param_specs[] = {
> 	{ "autocell",	Opt_autocell,	fs_parse_flag },
> 	{ "dyn",	Opt_dyn,	fs_parse_flag },
> 	{ "flock",	Opt_flock,	fs_parse_enum },
> 	{ "source",	Opt_source,	fs_parse_string },
>         {}
> };
> 
> 
> is a lot more obvious than:
> 
> static const struct fs_parameter_spec afs_param_specs[] = {
>         fsparam_flag  ("autocell",      Opt_autocell),
>         fsparam_flag  ("dyn",           Opt_dyn),
>         fsparam_enum  ("flock",         Opt_flock),
>         fsparam_string("source",        Opt_source),
>         {}
> };

Except that I want to be able to have something like
-       fsparam_enum   ("errors",             Opt_errors),
+       fsparam_enum   ("errors",             Opt_errors, gfs2_param_errors),
with
+static const struct fs_parameter_enum gfs2_param_errors[] = {
+       {"withdraw",   Opt_errors_withdraw },
+       {"panic",      Opt_errors_panic },
+       {}
+};
instead of having them all squashed into one array, as in
-static const struct fs_parameter_enum gfs2_param_enums[] = {
-       { Opt_quota,    "off",        Opt_quota_off },
-       { Opt_quota,    "account",    Opt_quota_account },
-       { Opt_quota,    "on",         Opt_quota_on },
-       { Opt_data,     "writeback",  Opt_data_writeback },
-       { Opt_data,     "ordered",    Opt_data_ordered },
-       { Opt_errors,   "withdraw",   Opt_errors_withdraw },
-       { Opt_errors,   "panic",      Opt_errors_panic },
...
 const struct fs_parameter_description gfs2_fs_parameters = {
        .name = "gfs2",
        .specs = gfs2_param_specs,
-       .enums = gfs2_param_enums,
 };

IOW, I want to kill ->enums thing.  And ->name is also trivial
to kill, at which point we are left with just what used to be
->specs.

Another thing is, struct fs_parameter_enum becomes pretty
much identical to struct constant_table and can be folded into it.

I have some experiments in that direction (very incomplete right
now) in #work.mount-parser-later; next cycle fodder, I'm afraid.

	Another thing is, consider something like "it's an
integer in range from 2 to 36".  Fairly useful in many cases,
and we could do helpers for that.  Except that they need a pointer
to helper-private data (the limits)...

	These macros somewhat isolate the filesystems until the
things settle down.  And one needs examples of conversions to
see what's missing - inventing a grand scheme out of thin air
doesn't work...
Christoph Hellwig Oct. 9, 2019, 6:01 p.m. UTC | #5
On Wed, Oct 09, 2019 at 05:03:10PM +0100, Al Viro wrote:
> Except that I want to be able to have something like
> -       fsparam_enum   ("errors",             Opt_errors),
> +       fsparam_enum   ("errors",             Opt_errors, gfs2_param_errors),
> with
> +static const struct fs_parameter_enum gfs2_param_errors[] = {
> +       {"withdraw",   Opt_errors_withdraw },
> +       {"panic",      Opt_errors_panic },
> +       {}
> +};
> instead of having them all squashed into one array, as in

Makes total sense and still fits the above scheme.

> IOW, I want to kill ->enums thing.  And ->name is also trivial
> to kill, at which point we are left with just what used to be
> ->specs.

Agreed.

> I have some experiments in that direction (very incomplete right
> now) in #work.mount-parser-later; next cycle fodder, I'm afraid.

I like that a lot, and feel like we really shouldn't do more
conversions until that ground work has been done
Al Viro Oct. 9, 2019, 6:22 p.m. UTC | #6
On Wed, Oct 09, 2019 at 11:01:02AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 05:03:10PM +0100, Al Viro wrote:
> > Except that I want to be able to have something like
> > -       fsparam_enum   ("errors",             Opt_errors),
> > +       fsparam_enum   ("errors",             Opt_errors, gfs2_param_errors),
> > with
> > +static const struct fs_parameter_enum gfs2_param_errors[] = {
> > +       {"withdraw",   Opt_errors_withdraw },
> > +       {"panic",      Opt_errors_panic },
> > +       {}
> > +};
> > instead of having them all squashed into one array, as in
> 
> Makes total sense and still fits the above scheme.
> 
> > IOW, I want to kill ->enums thing.  And ->name is also trivial
> > to kill, at which point we are left with just what used to be
> > ->specs.
> 
> Agreed.
> 
> > I have some experiments in that direction (very incomplete right
> > now) in #work.mount-parser-later; next cycle fodder, I'm afraid.
> 
> I like that a lot, and feel like we really shouldn't do more
> conversions until that ground work has been done

I'm not sure - for one thing, it's pretty much orthogonal to the
bulk of conversion; for another, I'm very sceptical about grand
schemes invented out of thin air in hope to cover everything in
the world, without finding out what _is_ there first.

Massaging the parser data structures can be done on top of the
other work just as well - the conflicts will be trivial to deal
with.  And I'm perfectly fine with having the parser stuff
go in last, just prior to -rc1, so resolution will be my
headache.

Alternatively, I can do never-rebased short branch right now,
with the parts of changes likely to cause conflicts, so that
xfs, nfs, etc. work could pull it and be done with that.

Note that e.g. conversion of fs_is_... from enum members to
functions would require zero changes in filesystems.  It would
allow to simplify them later on, but I would very much prefer
to do those extra helpers with converted codebase already
on hand.
Ian Kent Oct. 10, 2019, 12:59 a.m. UTC | #7
On Wed, 2019-10-09 at 07:48 -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 07:30:36PM +0800, Ian Kent wrote:
> > The mount-api doesn't have a "human unit" parse type yet so
> > the options that have values like "10k" etc. still need to
> > be converted by the fs.
> 
> Any reason you don't lift it to the the core mount api code?

I talked to David about that and there were some patches but
I didn't see them posted.

But it seems there may have been a reason for that, based on
Al's comments.

I'll leave this as it is for now.

Ian
Christoph Hellwig Dec. 6, 2019, 8:27 a.m. UTC | #8
On Wed, Oct 09, 2019 at 07:22:22PM +0100, Al Viro wrote:
> Massaging the parser data structures can be done on top of the
> other work just as well - the conflicts will be trivial to deal
> with.  And I'm perfectly fine with having the parser stuff
> go in last, just prior to -rc1, so resolution will be my
> headache.

So, it is that time of the cycle now.  Would you mind updating
the branch and feeding it to Linus?
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 9c1ce3d70c08..6a16750b1314 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -160,13 +160,13 @@  static const struct fs_parameter_description xfs_fs_parameters = {
 };
 
 STATIC int
-suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
+suffix_kstrtoint(const char *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
 	char	*value;
 	int	ret = 0;
 
-	value = match_strdup(s);
+	value = kstrdup(s, GFP_KERNEL);
 	if (!value)
 		return -ENOMEM;
 
@@ -191,6 +191,20 @@  suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 	return ret;
 }
 
+STATIC int
+match_kstrtoint(const substring_t *s, unsigned int base, int *res)
+{
+	const char	*value;
+	int ret;
+
+	value = match_strdup(s);
+	if (!value)
+		return -ENOMEM;
+	ret = suffix_kstrtoint(value, base, res);
+	kfree(value);
+	return ret;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -262,7 +276,7 @@  xfs_parseargs(
 				return -EINVAL;
 			break;
 		case Opt_logbsize:
-			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
+			if (match_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
 			break;
 		case Opt_logdev:
@@ -278,7 +292,7 @@  xfs_parseargs(
 				return -ENOMEM;
 			break;
 		case Opt_allocsize:
-			if (suffix_kstrtoint(args, 10, &iosize))
+			if (match_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
 			break;