fs_parse: handle optional arguments sanely

Don't bother with "mixed" options that would allow both the
form with and without argument (i.e. both -o foo and -o foo=bar).
Rather than trying to shove both into a single fs_parameter_spec,
allow having with-argument and no-argument specs with the same
name and teach fs_parse to handle that.

There are very few options of that sort, and they are actually
easier to handle that way - callers end up with less postprocessing.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

fs/ceph/super.c

@@ -179,8 +179,8 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
 	fsparam_flag_no ("copyfrom",			Opt_copyfrom),
 	fsparam_flag_no ("dcache",			Opt_dcache),
 	fsparam_flag_no ("dirstat",			Opt_dirstat),
-	__fsparam	(fs_param_is_string, "fsc",	Opt_fscache,
-			 fs_param_neg_with_no | fs_param_v_optional, NULL),
+	fsparam_flag_no	("fsc",				Opt_fscache), // fsc|nofsc
+	fsparam_string	("fsc",				Opt_fscache), // fsc=...
 	fsparam_flag_no ("ino32",			Opt_ino32),
 	fsparam_string	("mds_namespace",		Opt_mds_namespace),
 	fsparam_flag_no ("poolperm",			Opt_poolperm),

fs/fs_parser.c

@@ -46,19 +46,40 @@ int lookup_constant(const struct constant_table *tbl, const char *name, int not_
 }
 EXPORT_SYMBOL(lookup_constant);
 
+static inline bool is_flag(const struct fs_parameter_spec *p)
+{
+	return p->type == fs_param_is_flag;
+}
+
 static const struct fs_parameter_spec *fs_lookup_key(
 	const struct fs_parameter_spec *desc,
-	const char *name)
+	struct fs_parameter *param, bool *negated)
 {
-	const struct fs_parameter_spec *p;
-	if (!desc)
-		return NULL;
-
-	for (p = desc; p->name; p++)
-		if (strcmp(p->name, name) == 0)
+	const struct fs_parameter_spec *p, *other = NULL;
+	const char *name = param->key;
+	bool want_flag = param->type == fs_value_is_flag;
+
+	*negated = false;
+	for (p = desc; p->name; p++) {
+		if (strcmp(p->name, name) != 0)
+			continue;
+		if (likely(is_flag(p) == want_flag))
 			return p;
-
-	return NULL;
+		other = p;
+	}
+	if (want_flag) {
+		if (name[0] == 'n' && name[1] == 'o' && name[2]) {
+			for (p = desc; p->name; p++) {
+				if (strcmp(p->name, name + 2) != 0)
+					continue;
+				if (!(p->flags & fs_param_neg_with_no))
+					continue;
+				*negated = true;
+				return p;
+			}
+		}
+	}
+	return other;
 }
 
 /*
@@ -88,123 +109,77 @@ int __fs_parse(struct p_log *log,
 	const struct constant_table *e;
 	int ret = -ENOPARAM, b;
 
-	result->negated = false;
 	result->uint_64 = 0;
 
-	p = fs_lookup_key(desc, param->key);
-	if (!p) {
-		/* If we didn't find something that looks like "noxxx", see if
-		 * "xxx" takes the "no"-form negative - but only if there
-		 * wasn't an value.
-		 */
-		if (param->type != fs_value_is_flag)
-			goto unknown_parameter;
-		if (param->key[0] != 'n' || param->key[1] != 'o' || !param->key[2])
-			goto unknown_parameter;
-
-		p = fs_lookup_key(desc, param->key + 2);
-		if (!p)
-			goto unknown_parameter;
-		if (!(p->flags & fs_param_neg_with_no))
-			goto unknown_parameter;
-		result->boolean = false;
-		result->negated = true;
-	}
+	p = fs_lookup_key(desc, param, &result->negated);
+	if (!p)
+		return -ENOPARAM;
 
 	if (p->flags & fs_param_deprecated)
 		warn_plog(log, "Deprecated parameter '%s'", param->key);
 
-	if (result->negated)
-		goto okay;
-
-	/* Certain parameter types only take a string and convert it. */
-	switch (p->type) {
-	case __fs_param_wasnt_defined:
-		return -EINVAL;
-	case fs_param_is_u32:
-	case fs_param_is_u32_octal:
-	case fs_param_is_u32_hex:
-	case fs_param_is_s32:
-	case fs_param_is_u64:
-	case fs_param_is_enum:
-	case fs_param_is_string:
-		if (param->type == fs_value_is_string) {
-			if (p->flags & fs_param_v_optional)
-				break;
-			if (!*param->string)
-				goto bad_value;
-			break;
-		}
-		if (param->type == fs_value_is_flag) {
-			if (p->flags & fs_param_v_optional)
-				goto okay;
-		}
-		goto bad_value;
-	default:
-		break;
-	}
-
 	/* Try to turn the type we were given into the type desired by the
 	 * parameter and give an error if we can't.
 	 */
 	switch (p->type) {
+	case __fs_param_wasnt_defined:
+		return -EINVAL;
 	case fs_param_is_flag:
 		if (param->type != fs_value_is_flag)
 			return inval_plog(log, "Unexpected value for '%s'",
 				      param->key);
-		result->boolean = true;
+		result->boolean = !result->negated;
 		goto okay;
-
 	case fs_param_is_bool:
-		switch (param->type) {
-		case fs_value_is_flag:
-			result->boolean = true;
-			goto okay;
-		case fs_value_is_string:
-			if (param->size == 0) {
-				result->boolean = true;
-				goto okay;
-			}
-			b = lookup_constant(bool_names, param->string, -1);
-			if (b == -1)
-				goto bad_value;
-			result->boolean = b;
-			goto okay;
-		default:
+		if (param->type != fs_value_is_string)
 			goto bad_value;
-		}
-
+		b = lookup_constant(bool_names, param->string, -1);
+		if (b == -1)
+			goto bad_value;
+		result->boolean = b;
+		goto okay;
 	case fs_param_is_u32:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		ret = kstrtouint(param->string, 0, &result->uint_32);
 		goto maybe_okay;
 	case fs_param_is_u32_octal:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		ret = kstrtouint(param->string, 8, &result->uint_32);
 		goto maybe_okay;
 	case fs_param_is_u32_hex:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		ret = kstrtouint(param->string, 16, &result->uint_32);
 		goto maybe_okay;
 	case fs_param_is_s32:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		ret = kstrtoint(param->string, 0, &result->int_32);
 		goto maybe_okay;
 	case fs_param_is_u64:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		ret = kstrtoull(param->string, 0, &result->uint_64);
 		goto maybe_okay;
-
 	case fs_param_is_enum:
+		if (param->type != fs_value_is_string)
+			goto bad_value;
 		e = __lookup_constant(p->data, param->string);
 		if (e) {
 			result->uint_32 = e->value;
 			goto okay;
 		}
 		goto bad_value;
-
 	case fs_param_is_string:
+		if (param->type != fs_value_is_string || !*param->string)
+			goto bad_value;
 		goto okay;
 	case fs_param_is_blob:
 		if (param->type != fs_value_is_blob)
 			goto bad_value;
 		goto okay;
-
 	case fs_param_is_fd: {
 		switch (param->type) {
 		case fs_value_is_string:
@@ -221,7 +196,6 @@ int __fs_parse(struct p_log *log,
 			goto bad_value;
 		goto maybe_okay;
 	}
-
 	case fs_param_is_blockdev:
 	case fs_param_is_path:
 		goto okay;
@@ -237,8 +211,6 @@ int __fs_parse(struct p_log *log,
 
 bad_value:
 	return inval_plog(log, "Bad value for '%s'", param->key);
-unknown_parameter:
-	return -ENOPARAM;
 }
 EXPORT_SYMBOL(__fs_parse);
 
@@ -382,6 +354,8 @@ bool fs_validate_description(const char *name,
 		/* Check for duplicate parameter names */
 		for (p2 = desc; p2 < param; p2++) {
 			if (strcmp(param->name, p2->name) == 0) {
+				if (is_flag(param) != is_flag(p2))
+					continue;
 				pr_err("VALIDATE %s: PARAM[%s]: Duplicate\n",
 				       name, param->name);
 				good = false;

fs/gfs2/ops_fstype.c

@@ -1250,6 +1250,7 @@ enum gfs2_param {
 	Opt_upgrade,
 	Opt_acl,
 	Opt_quota,
+	Opt_quota_flag,
 	Opt_suiddir,
 	Opt_data,
 	Opt_meta,
@@ -1264,26 +1265,13 @@ enum gfs2_param {
 	Opt_loccookie,
 };
 
-enum opt_quota {
-	Opt_quota_unset = 0,
-	Opt_quota_off,
-	Opt_quota_account,
-	Opt_quota_on,
-};
-
 static const struct constant_table gfs2_param_quota[] = {
-	{"off",        Opt_quota_off },
-	{"account",    Opt_quota_account },
-	{"on",         Opt_quota_on },
+	{"off",        GFS2_QUOTA_OFF},
+	{"account",    GFS2_QUOTA_ACCOUNT},
+	{"on",         GFS2_QUOTA_ON},
 	{}
 };
 
-static const unsigned int opt_quota_values[] = {
-	[Opt_quota_off]     = GFS2_QUOTA_OFF,
-	[Opt_quota_account] = GFS2_QUOTA_ACCOUNT,
-	[Opt_quota_on]      = GFS2_QUOTA_ON,
-};
-
 enum opt_data {
 	Opt_data_writeback = GFS2_DATA_WRITEBACK,
 	Opt_data_ordered   = GFS2_DATA_ORDERED,
@@ -1331,8 +1319,8 @@ static const struct fs_parameter_spec gfs2_fs_parameters[] = {
 	fsparam_flag_no("rgrplvb",            Opt_rgrplvb),
 	fsparam_flag_no("loccookie",          Opt_loccookie),
 	/* quota can be a flag or an enum so it gets special treatment */
-	__fsparam(fs_param_is_enum, "quota", Opt_quota,
-		fs_param_neg_with_no|fs_param_v_optional, gfs2_param_quota),
+	fsparam_flag_no("quota",	      Opt_quota_flag),
+	fsparam_enum("quota",		      Opt_quota, gfs2_param_quota),
 	{}
 };
 
@@ -1380,17 +1368,11 @@ static int gfs2_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_acl:
 		args->ar_posix_acl = result.boolean;
 		break;
+	case Opt_quota_flag:
+		args->ar_quota = result.negated ? GFS2_QUOTA_OFF : GFS2_QUOTA_ON;
+		break;
 	case Opt_quota:
-		/* The quota option can be a flag or an enum. A non-zero int_32
-		   result means that we have an enum index. Otherwise we have
-		   to rely on the 'negated' flag to tell us whether 'quota' or
-		   'noquota' was specified. */
-		if (result.negated)
-			args->ar_quota = GFS2_QUOTA_OFF;
-		else if (result.int_32 > 0)
-			args->ar_quota = opt_quota_values[result.int_32];
-		else
-			args->ar_quota = GFS2_QUOTA_ON;
+		args->ar_quota = result.int_32;
 		break;
 	case Opt_suiddir:
 		args->ar_suiddir = result.boolean;

fs/nfs/fs_context.c

@@ -45,6 +45,7 @@ enum nfs_param {
 	Opt_cto,
 	Opt_fg,
 	Opt_fscache,
+	Opt_fscache_flag,
 	Opt_hard,
 	Opt_intr,
 	Opt_local_lock,
@@ -125,8 +126,8 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
 	fsparam_string("clientaddr",	Opt_clientaddr),
 	fsparam_flag_no("cto",		Opt_cto),
 	fsparam_flag  ("fg",		Opt_fg),
-	__fsparam(fs_param_is_string, "fsc",		Opt_fscache,
-		  fs_param_neg_with_no|fs_param_v_optional, NULL),
+	fsparam_flag_no("fsc",		Opt_fscache_flag),
+	fsparam_string("fsc",		Opt_fscache),
 	fsparam_flag  ("hard",		Opt_hard),
 	__fsparam(fs_param_is_flag, "intr",		Opt_intr,
 		  fs_param_neg_with_no|fs_param_deprecated, NULL),
@@ -537,14 +538,19 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		else
 			ctx->flags &= ~NFS_MOUNT_NORESVPORT;
 		break;
-	case Opt_fscache:
-		kfree(ctx->fscache_uniq);
-		ctx->fscache_uniq = param->string;
-		param->string = NULL;
+	case Opt_fscache_flag:
 		if (result.negated)
 			ctx->options &= ~NFS_OPTION_FSCACHE;
 		else
 			ctx->options |= NFS_OPTION_FSCACHE;
+		kfree(ctx->fscache_uniq);
+		ctx->fscache_uniq = NULL;
+		break;
+	case Opt_fscache:
+		ctx->options |= NFS_OPTION_FSCACHE;
+		kfree(ctx->fscache_uniq);
+		ctx->fscache_uniq = param->string;
+		param->string = NULL;
 		break;
 	case Opt_migration:
 		if (result.negated)

include/linux/fs_parser.h

@@ -49,7 +49,6 @@ struct fs_parameter_spec {
 	u8			opt;	/* Option number (returned by fs_parse()) */
 	enum fs_parameter_type	type:8;	/* The desired parameter type */
 	unsigned short		flags;
-#define fs_param_v_optional	0x0001	/* The value is optional */
 #define fs_param_neg_with_no	0x0002	/* "noxxx" is negative param */
 #define fs_param_neg_with_empty	0x0004	/* "xxx=" is negative param */
 #define fs_param_deprecated	0x0008	/* The param is deprecated */