Re: [PATCH v3] ceph: Convert ceph to use the new mount API

From: Al Viro <viro@zeniv.linux.org.uk>
To: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>,
	David Howells <dhowells@redhat.com>,
	Ceph Development <ceph-devel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	"Yan, Zheng" <zyan@redhat.com>
Subject: Re: [PATCH v3] ceph: Convert ceph to use the new mount API
Date: Sat, 7 Sep 2019 16:07:34 +0100	[thread overview]
Message-ID: <20190907150734.GB1131@ZenIV.linux.org.uk> (raw)
In-Reply-To: <7a72bf67b17f78398604270a2cbfe5d145686377.camel@kernel.org>

On Fri, Sep 06, 2019 at 04:10:24PM -0400, Jeff Layton wrote:
> > >         case Opt_queue_depth:
> > > -               if (intval < 1) {
> > > -                       pr_err("queue_depth out of range\n");
> > > -                       return -EINVAL;
> > > -               }
> > > -               pctx->opts->queue_depth = intval;
> > > +               if (result.uint_32 < 1)
> > > +                       goto out_of_range;
> > > +               opts->queue_depth = result.uint_32;

FWIW, I wonder if something like fsparam_int_range() would be
useful, both here and in other places.

NOTE: this is not going to happen until we get rid of trying to
enumerate those "types"; enum fs_parameter_type as it is now
is an invitation for trouble.

What I want to get at is a situation when fs_parameter_spec
"type" is a *method*, not an enumerator.  I'm not entirely
sure what would the right calling conventions be, though.

But fs_parse() switch is not sustainable - we can't keep
it long-term.  A really straigtforward approach would be
something along the lines of

bool is_string_param(const struct fs_parameter_spec *p,
	            struct fs_parameter *param)
{
	if (param->type != fs_value_is_string)
		return false;
	if (param->string)
		return true;
	return p->flags & fs_param_v_optional;
}

int fs_param_is_string(struct fs_context *fc,
		    const struct fs_parameter_spec *p,
	            struct fs_parameter *param,
		    struct fs_parse_result *result)
{
	if (is_string_param(p, param))
		return 0;
	return fs_param_bad(fc, param);
}

int fs_param_is_s32(struct fs_context *fc,
		    const struct fs_parameter_spec *p,
	            struct fs_parameter *param,
		    struct fs_parse_result *result)
{
	if (is_string_param(p, param)) {
		const char *s = param->string;
		result->int_32 = 0;
		if (!s || kstrtoint(s, 0, &result->int_32) == 0)
			return 0;
	}
	return fs_param_bad(fc, param);
}

int fs_param_is_blob(struct fs_context *fc,
		    const struct fs_parameter_spec *p,
	            struct fs_parameter *param,
		    struct fs_parse_result *result)
{
	return param->type == fs_value_is_blob ? 0 : fs_param_bad(fc, param);
}

int fs_param_is_fd(struct fs_context *fc,
		    const struct fs_parameter_spec *p,
	            struct fs_parameter *param,
		    struct fs_parse_result *result)
{
	if (param->type == fs_value_is_file) {
		result->uint_32 = param->dirfd;
                if (result->uint_32 <= INT_MAX)
			return 0;
	} else if (is_string_param(p, param)) {
		const char *s = param->string;

		if (s && kstrtouint(param->string, 0, &result->uint_32) == 0 &&
                    result->uint_32 <= INT_MAX)
			return 0;
        }
	return fs_param_bad(fc, param);
}

etc., but error reporting is clumsy that way ;-/