[PATCH V2] fs_parser: remove fs_parameter_description name field

[PATCH V2] fs_parser: remove fs_parameter_description name field

[Eric Sandeen]

There doesn't seem to be a strong reason to have a copy of the
filesystem name string in the fs_parameter_description structure;
it's easy enough to get the name from the fs_type, and using it
instead ensures consistency across messages (for example,
vfs_parse_fs_param() already uses fc->fs_type->name for the error
messages, because it doesn't have the fs_parameter_description).

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
---

V2: add more recently converted filesystems and fix kernel docs

(This also fixes ffs_fs_fs_parameters which mistakenly named itself
"kAFS" - which shows why redundant copies of information is a bad
plan.)

diff --git a/Documentation/filesystems/mount_api.txt b/Documentation/filesystems/mount_api.txt
index 00ff0cf..7e09dc3 100644
--- a/Documentation/filesystems/mount_api.txt
+++ b/Documentation/filesystems/mount_api.txt
@@ -519,7 +519,6 @@ Parameters are described using structures defined in linux/fs_parser.h.
 There's a core description struct that links everything together:
 
 	struct fs_parameter_description {
-		const char	name[16];
 		const struct fs_parameter_spec *specs;
 		const struct fs_parameter_enum *enums;
 	};
@@ -535,19 +534,13 @@ For example:
 	};
 
 	static const struct fs_parameter_description afs_fs_parameters = {
-		.name		= "kAFS",
 		.specs		= afs_param_specs,
 		.enums		= afs_param_enums,
 	};
 
 The members are as follows:
 
- (1) const char name[16];
-
-     The name to be used in error messages generated by the parse helper
-     functions.
-
- (2) const struct fs_parameter_specification *specs;
+ (1) const struct fs_parameter_specification *specs;
 
      Table of parameter specifications, terminated with a null entry, where the
      entries are of type:
@@ -626,7 +619,7 @@ The members are as follows:
      of arguments to specify the type and the flags for anything that doesn't
      match one of the above macros.
 
- (6) const struct fs_parameter_enum *enums;
+ (2) const struct fs_parameter_enum *enums;
 
      Table of enum value names to integer mappings, terminated with a null
      entry.  This is of type:
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index 9b1586b..36ce5d0 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -592,7 +592,6 @@ enum {
 };
 
 static const struct fs_parameter_description spufs_fs_parameters = {
-	.name		= "spufs",
 	.specs		= spufs_param_specs,
 };
 
diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index 70139d0..b3a6d13 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -216,7 +216,6 @@ static int hypfs_release(struct inode *inode, struct file *filp)
 };
 
 static const struct fs_parameter_description hypfs_fs_parameters = {
-	.name		= "hypfs",
 	.specs		= hypfs_param_specs,
 };
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2e3b06d..f145594 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2045,7 +2045,6 @@ enum rdt_param {
 };
 
 static const struct fs_parameter_description rdt_fs_parameters = {
-	.name		= "rdt",
 	.specs		= rdt_param_specs,
 };
 
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2b18456..b33f147 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -864,7 +864,6 @@ enum {
 };
 
 static const struct fs_parameter_description rbd_parameters = {
-	.name		= "rbd",
 	.specs		= rbd_param_specs,
 };
 
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ce1d023..14ef94f 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1497,7 +1497,6 @@ enum {
 };
 
 static const struct fs_parameter_description ffs_fs_fs_parameters = {
-	.name		= "kAFS",
 	.specs		= ffs_fs_param_specs,
 };
 
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 488641b..8edbd87 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -42,7 +42,6 @@
 
 struct file_system_type afs_fs_type = {
 	.owner			= THIS_MODULE,
-	.name			= "afs",
 	.init_fs_context	= afs_init_fs_context,
 	.parameters		= &afs_fs_parameters,
 	.kill_sb		= afs_kill_super,
@@ -90,7 +89,6 @@ enum afs_param {
 };
 
 static const struct fs_parameter_description afs_fs_parameters = {
-	.name		= "kAFS",
 	.specs		= afs_param_specs,
 	.enums		= afs_param_enums,
 };
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9c9a7c6..c164256 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -199,7 +199,6 @@ enum ceph_recover_session_mode {
 };
 
 static const struct fs_parameter_description ceph_mount_parameters = {
-	.name           = "ceph",
 	.specs          = ceph_mount_param_specs,
 	.enums		= ceph_mount_param_enums,
 };
diff --git a/fs/filesystems.c b/fs/filesystems.c
index 9135646..77bf5f9 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -74,7 +74,8 @@ int register_filesystem(struct file_system_type * fs)
 	int res = 0;
 	struct file_system_type ** p;
 
-	if (fs->parameters && !fs_validate_description(fs->parameters))
+	if (fs->parameters &&
+	    !fs_validate_description(fs->name, fs->parameters))
 		return -EINVAL;
 
 	BUG_ON(strchr(fs->name, '.'));
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index d1930ad..866c71b 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -111,7 +111,7 @@ int fs_parse(struct fs_context *fc,
 
 	if (p->flags & fs_param_deprecated)
 		warnf(fc, "%s: Deprecated parameter '%s'",
-		      desc->name, param->key);
+		      fc->fs_type->name, param->key);
 
 	if (result->negated)
 		goto okay;
@@ -147,7 +147,7 @@ int fs_parse(struct fs_context *fc,
 		if (param->type != fs_value_is_flag &&
 		    (param->type != fs_value_is_string || result->has_value))
 			return invalf(fc, "%s: Unexpected value for '%s'",
-				      desc->name, param->key);
+				      fc->fs_type->name, param->key);
 		result->boolean = true;
 		goto okay;
 
@@ -237,7 +237,8 @@ int fs_parse(struct fs_context *fc,
 	return p->opt;
 
 bad_value:
-	return invalf(fc, "%s: Bad value for '%s'", desc->name, param->key);
+	return invalf(fc, "%s: Bad value for '%s'",
+		      fc->fs_type->name, param->key);
 unknown_parameter:
 	return -ENOPARAM;
 }
@@ -357,22 +358,16 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
  * fs_validate_description - Validate a parameter description
  * @desc: The parameter description to validate.
  */
-bool fs_validate_description(const struct fs_parameter_description *desc)
+bool fs_validate_description(const char *name,
+	const struct fs_parameter_description *desc)
 {
 	const struct fs_parameter_spec *param, *p2;
 	const struct fs_parameter_enum *e;
-	const char *name = desc->name;
 	unsigned int nr_params = 0;
 	bool good = true, enums = false;
 
 	pr_notice("*** VALIDATE %s ***\n", name);
 
-	if (!name[0]) {
-		pr_err("VALIDATE Parser: No name\n");
-		name = "Unknown";
-		good = false;
-	}
-
 	if (desc->specs) {
 		for (param = desc->specs; param->name; param++) {
 			enum fs_parameter_type t = param->type;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 16aec32..5a01daa 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -463,7 +463,6 @@ enum {
 };
 
 static const struct fs_parameter_description fuse_fs_parameters = {
-	.name		= "fuse",
 	.specs		= fuse_param_specs,
 };
 
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index e8b7b0c..d406729 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1328,7 +1328,6 @@ enum opt_errors {
 };
 
 static const struct fs_parameter_description gfs2_fs_parameters = {
-	.name = "gfs2",
 	.specs = gfs2_param_specs,
 	.enums = gfs2_param_enums,
 };
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index d5c2a31..2ac0143 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -85,7 +85,6 @@ enum hugetlb_param {
 };
 
 static const struct fs_parameter_description hugetlb_fs_parameters = {
-	.name		= "hugetlbfs",
 	.specs		= hugetlb_param_specs,
 };
 
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 0e6406c..55d44aa 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -185,7 +185,6 @@ enum {
 };
 
 const struct fs_parameter_description jffs2_fs_parameters = {
-	.name		= "jffs2",
 	.specs		= jffs2_param_specs,
 	.enums		= jffs2_param_enums,
 };
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 0b7c8df..c447654 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -48,7 +48,6 @@ enum proc_param {
 };
 
 static const struct fs_parameter_description proc_fs_parameters = {
-	.name		= "proc",
 	.specs		= proc_param_specs,
 };
 
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index d82636e..bb7ab56 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -187,7 +187,6 @@ enum ramfs_param {
 };
 
 const struct fs_parameter_description ramfs_fs_parameters = {
-	.name		= "ramfs",
 	.specs		= ramfs_param_specs,
 };
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d9ae27d..ee23a2b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -107,7 +107,6 @@ enum {
 };
 
 static const struct fs_parameter_description xfs_fs_parameters = {
-	.name		= "xfs",
 	.specs		= xfs_param_specs,
 };
 
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index dee140d..090a2ed 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -62,7 +62,6 @@ struct fs_parameter_enum {
 };
 
 struct fs_parameter_description {
-	const char	name[16];		/* Name for logging purposes */
 	const struct fs_parameter_spec *specs;	/* List of param specifications */
 	const struct fs_parameter_enum *enums;	/* Enum values */
 };
@@ -97,12 +96,14 @@ extern int __lookup_constant(const struct constant_table tbl[], size_t tbl_size,
 #ifdef CONFIG_VALIDATE_FS_PARSER
 extern bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 				    int low, int high, int special);
-extern bool fs_validate_description(const struct fs_parameter_description *desc);
+extern bool fs_validate_description(const char *name,
+				    const struct fs_parameter_description *desc);
 #else
 static inline bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 					   int low, int high, int special)
 { return true; }
-static inline bool fs_validate_description(const struct fs_parameter_description *desc)
+static inline bool fs_validate_description(const char *name,
+					   const struct fs_parameter_description *desc)
 { return true; }
 #endif
 
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index ecf42be..9608aa4 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -593,7 +593,6 @@ enum {
 };
 
 static const struct fs_parameter_description bpf_fs_parameters = {
-	.name		= "bpf",
 	.specs		= bpf_param_specs,
 };
 
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 09f3a41..e711a43 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -900,7 +900,6 @@ enum cgroup1_param {
 };
 
 const struct fs_parameter_description cgroup1_fs_parameters = {
-	.name		= "cgroup1",
 	.specs		= cgroup1_param_specs,
 };
 
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 735af8f..d86d441 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1823,7 +1823,6 @@ enum cgroup2_param {
 };
 
 static const struct fs_parameter_description cgroup2_fs_parameters = {
-	.name		= "cgroup2",
 	.specs		= cgroup2_param_specs,
 };
 
diff --git a/mm/shmem.c b/mm/shmem.c
index 165fa63..b6dc807 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3401,7 +3401,6 @@ enum shmem_param {
 };
 
 const struct fs_parameter_description shmem_fs_parameters = {
-	.name		= "tmpfs",
 	.specs		= shmem_param_specs,
 	.enums		= shmem_param_enums,
 };
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index a9d6c97..895a563 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -291,7 +291,6 @@ enum {
 };
 
 static const struct fs_parameter_description ceph_parameters = {
-        .name           = "libceph",
         .specs          = ceph_param_specs,
 };
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 116b4d6..54f3463 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2818,7 +2818,6 @@ static int selinux_fs_context_dup(struct fs_context *fc,
 };
 
 static const struct fs_parameter_description selinux_fs_parameters = {
-	.name		= "SELinux",
 	.specs		= selinux_param_specs,
 };
 
@@ -7145,7 +7144,7 @@ static __init int selinux_init(void)
 	else
 		pr_debug("SELinux:  Starting in permissive mode\n");
 
-	fs_validate_description(&selinux_fs_parameters);
+	fs_validate_description("selinux", &selinux_fs_parameters);
 
 	return 0;
 }
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ecea41c..646c0b4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -689,7 +689,6 @@ static int smack_fs_context_dup(struct fs_context *fc,
 };
 
 static const struct fs_parameter_description smack_fs_parameters = {
-	.name		= "smack",
 	.specs		= smack_param_specs,
 };
 

[PATCH V3] fs_parser: remove fs_parameter_description name field

[Eric Sandeen]

There doesn't seem to be a strong reason to have a copy of the
filesystem name string in the fs_parameter_description structure;
it's easy enough to get the name from the fs_type, and using it
instead ensures consistency across messages (for example,
vfs_parse_fs_param() already uses fc->fs_type->name for the error
messages, because it doesn't have the fs_parameter_description).

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Acked-by: David Howells <dhowells@redhat.com>
---

V2: add more recently converted filesystems and fix kernel docs
V3: *sigh* add name back to afs fs_type, no idea how I botched that

(This also fixes ffs_fs_fs_parameters which mistakenly named itself
"kAFS" - which shows why redundant copies of information is a bad
plan.)

diff --git a/Documentation/filesystems/mount_api.txt b/Documentation/filesystems/mount_api.txt
index 00ff0cf..7e09dc3 100644
--- a/Documentation/filesystems/mount_api.txt
+++ b/Documentation/filesystems/mount_api.txt
@@ -519,7 +519,6 @@ Parameters are described using structures defined in linux/fs_parser.h.
 There's a core description struct that links everything together:
 
 	struct fs_parameter_description {
-		const char	name[16];
 		const struct fs_parameter_spec *specs;
 		const struct fs_parameter_enum *enums;
 	};
@@ -535,19 +534,13 @@ For example:
 	};
 
 	static const struct fs_parameter_description afs_fs_parameters = {
-		.name		= "kAFS",
 		.specs		= afs_param_specs,
 		.enums		= afs_param_enums,
 	};
 
 The members are as follows:
 
- (1) const char name[16];
-
-     The name to be used in error messages generated by the parse helper
-     functions.
-
- (2) const struct fs_parameter_specification *specs;
+ (1) const struct fs_parameter_specification *specs;
 
      Table of parameter specifications, terminated with a null entry, where the
      entries are of type:
@@ -626,7 +619,7 @@ The members are as follows:
      of arguments to specify the type and the flags for anything that doesn't
      match one of the above macros.
 
- (6) const struct fs_parameter_enum *enums;
+ (2) const struct fs_parameter_enum *enums;
 
      Table of enum value names to integer mappings, terminated with a null
      entry.  This is of type:
diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c
index 9b1586b..36ce5d0 100644
--- a/arch/powerpc/platforms/cell/spufs/inode.c
+++ b/arch/powerpc/platforms/cell/spufs/inode.c
@@ -592,7 +592,6 @@ enum {
 };
 
 static const struct fs_parameter_description spufs_fs_parameters = {
-	.name		= "spufs",
 	.specs		= spufs_param_specs,
 };
 
diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index 70139d0..b3a6d13 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -216,7 +216,6 @@ static int hypfs_release(struct inode *inode, struct file *filp)
 };
 
 static const struct fs_parameter_description hypfs_fs_parameters = {
-	.name		= "hypfs",
 	.specs		= hypfs_param_specs,
 };
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2e3b06d..f145594 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2045,7 +2045,6 @@ enum rdt_param {
 };
 
 static const struct fs_parameter_description rdt_fs_parameters = {
-	.name		= "rdt",
 	.specs		= rdt_param_specs,
 };
 
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2b18456..b33f147 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -864,7 +864,6 @@ enum {
 };
 
 static const struct fs_parameter_description rbd_parameters = {
-	.name		= "rbd",
 	.specs		= rbd_param_specs,
 };
 
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index ce1d023..14ef94f 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1497,7 +1497,6 @@ enum {
 };
 
 static const struct fs_parameter_description ffs_fs_fs_parameters = {
-	.name		= "kAFS",
 	.specs		= ffs_fs_param_specs,
 };
 
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 488641b..8edbd87 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -90,7 +89,6 @@ enum afs_param {
 };
 
 static const struct fs_parameter_description afs_fs_parameters = {
-	.name		= "kAFS",
 	.specs		= afs_param_specs,
 	.enums		= afs_param_enums,
 };
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9c9a7c6..c164256 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -199,7 +199,6 @@ enum ceph_recover_session_mode {
 };
 
 static const struct fs_parameter_description ceph_mount_parameters = {
-	.name           = "ceph",
 	.specs          = ceph_mount_param_specs,
 	.enums		= ceph_mount_param_enums,
 };
diff --git a/fs/filesystems.c b/fs/filesystems.c
index 9135646..77bf5f9 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -74,7 +74,8 @@ int register_filesystem(struct file_system_type * fs)
 	int res = 0;
 	struct file_system_type ** p;
 
-	if (fs->parameters && !fs_validate_description(fs->parameters))
+	if (fs->parameters &&
+	    !fs_validate_description(fs->name, fs->parameters))
 		return -EINVAL;
 
 	BUG_ON(strchr(fs->name, '.'));
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index d1930ad..866c71b 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -111,7 +111,7 @@ int fs_parse(struct fs_context *fc,
 
 	if (p->flags & fs_param_deprecated)
 		warnf(fc, "%s: Deprecated parameter '%s'",
-		      desc->name, param->key);
+		      fc->fs_type->name, param->key);
 
 	if (result->negated)
 		goto okay;
@@ -147,7 +147,7 @@ int fs_parse(struct fs_context *fc,
 		if (param->type != fs_value_is_flag &&
 		    (param->type != fs_value_is_string || result->has_value))
 			return invalf(fc, "%s: Unexpected value for '%s'",
-				      desc->name, param->key);
+				      fc->fs_type->name, param->key);
 		result->boolean = true;
 		goto okay;
 
@@ -237,7 +237,8 @@ int fs_parse(struct fs_context *fc,
 	return p->opt;
 
 bad_value:
-	return invalf(fc, "%s: Bad value for '%s'", desc->name, param->key);
+	return invalf(fc, "%s: Bad value for '%s'",
+		      fc->fs_type->name, param->key);
 unknown_parameter:
 	return -ENOPARAM;
 }
@@ -357,22 +358,16 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
  * fs_validate_description - Validate a parameter description
  * @desc: The parameter description to validate.
  */
-bool fs_validate_description(const struct fs_parameter_description *desc)
+bool fs_validate_description(const char *name,
+	const struct fs_parameter_description *desc)
 {
 	const struct fs_parameter_spec *param, *p2;
 	const struct fs_parameter_enum *e;
-	const char *name = desc->name;
 	unsigned int nr_params = 0;
 	bool good = true, enums = false;
 
 	pr_notice("*** VALIDATE %s ***\n", name);
 
-	if (!name[0]) {
-		pr_err("VALIDATE Parser: No name\n");
-		name = "Unknown";
-		good = false;
-	}
-
 	if (desc->specs) {
 		for (param = desc->specs; param->name; param++) {
 			enum fs_parameter_type t = param->type;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 16aec32..5a01daa 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -463,7 +463,6 @@ enum {
 };
 
 static const struct fs_parameter_description fuse_fs_parameters = {
-	.name		= "fuse",
 	.specs		= fuse_param_specs,
 };
 
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index e8b7b0c..d406729 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1328,7 +1328,6 @@ enum opt_errors {
 };
 
 static const struct fs_parameter_description gfs2_fs_parameters = {
-	.name = "gfs2",
 	.specs = gfs2_param_specs,
 	.enums = gfs2_param_enums,
 };
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index d5c2a31..2ac0143 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -85,7 +85,6 @@ enum hugetlb_param {
 };
 
 static const struct fs_parameter_description hugetlb_fs_parameters = {
-	.name		= "hugetlbfs",
 	.specs		= hugetlb_param_specs,
 };
 
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 0e6406c..55d44aa 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -185,7 +185,6 @@ enum {
 };
 
 const struct fs_parameter_description jffs2_fs_parameters = {
-	.name		= "jffs2",
 	.specs		= jffs2_param_specs,
 	.enums		= jffs2_param_enums,
 };
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 0b7c8df..c447654 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -48,7 +48,6 @@ enum proc_param {
 };
 
 static const struct fs_parameter_description proc_fs_parameters = {
-	.name		= "proc",
 	.specs		= proc_param_specs,
 };
 
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index d82636e..bb7ab56 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -187,7 +187,6 @@ enum ramfs_param {
 };
 
 const struct fs_parameter_description ramfs_fs_parameters = {
-	.name		= "ramfs",
 	.specs		= ramfs_param_specs,
 };
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d9ae27d..ee23a2b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -107,7 +107,6 @@ enum {
 };
 
 static const struct fs_parameter_description xfs_fs_parameters = {
-	.name		= "xfs",
 	.specs		= xfs_param_specs,
 };
 
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index dee140d..090a2ed 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -62,7 +62,6 @@ struct fs_parameter_enum {
 };
 
 struct fs_parameter_description {
-	const char	name[16];		/* Name for logging purposes */
 	const struct fs_parameter_spec *specs;	/* List of param specifications */
 	const struct fs_parameter_enum *enums;	/* Enum values */
 };
@@ -97,12 +96,14 @@ extern int __lookup_constant(const struct constant_table tbl[], size_t tbl_size,
 #ifdef CONFIG_VALIDATE_FS_PARSER
 extern bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 				    int low, int high, int special);
-extern bool fs_validate_description(const struct fs_parameter_description *desc);
+extern bool fs_validate_description(const char *name,
+				    const struct fs_parameter_description *desc);
 #else
 static inline bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 					   int low, int high, int special)
 { return true; }
-static inline bool fs_validate_description(const struct fs_parameter_description *desc)
+static inline bool fs_validate_description(const char *name,
+					   const struct fs_parameter_description *desc)
 { return true; }
 #endif
 
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index ecf42be..9608aa4 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -593,7 +593,6 @@ enum {
 };
 
 static const struct fs_parameter_description bpf_fs_parameters = {
-	.name		= "bpf",
 	.specs		= bpf_param_specs,
 };
 
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 09f3a41..e711a43 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -900,7 +900,6 @@ enum cgroup1_param {
 };
 
 const struct fs_parameter_description cgroup1_fs_parameters = {
-	.name		= "cgroup1",
 	.specs		= cgroup1_param_specs,
 };
 
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 735af8f..d86d441 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1823,7 +1823,6 @@ enum cgroup2_param {
 };
 
 static const struct fs_parameter_description cgroup2_fs_parameters = {
-	.name		= "cgroup2",
 	.specs		= cgroup2_param_specs,
 };
 
diff --git a/mm/shmem.c b/mm/shmem.c
index 165fa63..b6dc807 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3401,7 +3401,6 @@ enum shmem_param {
 };
 
 const struct fs_parameter_description shmem_fs_parameters = {
-	.name		= "tmpfs",
 	.specs		= shmem_param_specs,
 	.enums		= shmem_param_enums,
 };
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index a9d6c97..895a563 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -291,7 +291,6 @@ enum {
 };
 
 static const struct fs_parameter_description ceph_parameters = {
-        .name           = "libceph",
         .specs          = ceph_param_specs,
 };
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 116b4d6..54f3463 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2818,7 +2818,6 @@ static int selinux_fs_context_dup(struct fs_context *fc,
 };
 
 static const struct fs_parameter_description selinux_fs_parameters = {
-	.name		= "SELinux",
 	.specs		= selinux_param_specs,
 };
 
@@ -7145,7 +7144,7 @@ static __init int selinux_init(void)
 	else
 		pr_debug("SELinux:  Starting in permissive mode\n");
 
-	fs_validate_description(&selinux_fs_parameters);
+	fs_validate_description("selinux", &selinux_fs_parameters);
 
 	return 0;
 }
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ecea41c..646c0b4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -689,7 +689,6 @@ static int smack_fs_context_dup(struct fs_context *fc,
 };
 
 static const struct fs_parameter_description smack_fs_parameters = {
-	.name		= "smack",
 	.specs		= smack_param_specs,
 };
 

Re: [PATCH V2] fs_parser: remove fs_parameter_description name field

[Al Viro]

On Fri, Dec 06, 2019 at 10:31:57AM -0600, Eric Sandeen wrote:
> There doesn't seem to be a strong reason to have a copy of the
> filesystem name string in the fs_parameter_description structure;
> it's easy enough to get the name from the fs_type, and using it
> instead ensures consistency across messages (for example,
> vfs_parse_fs_param() already uses fc->fs_type->name for the error
> messages, because it doesn't have the fs_parameter_description).

Arrgh...  That used to be fine.  Now we have this:
static int rbd_parse_param(struct fs_parameter *param,
                            struct rbd_parse_opts_ctx *pctx)
{
        struct rbd_options *opt = pctx->opts;
        struct fs_parse_result result; 
        int token, ret;

        ret = ceph_parse_param(param, pctx->copts, NULL);
        if (ret != -ENOPARAM)
                return ret;

        token = fs_parse(NULL, rbd_parameters, param, &result);
			 ^^^^

	Cthulhu damn it...  And yes, that crap used to work.
Frankly, I'm tempted to allocate fs_context in there (in
rbd_parse_options(), or in rbd_add_parse_args()) - we've other
oddities due to that...

	Alternatively, we could provide __fs_parse() that
would take name as a separate argument and accepted NULL fc,
with fs_parse() being a wrapper for that.

*grumble*

Re: [PATCH V2] fs_parser: remove fs_parameter_description name field

[Eric Sandeen]

On 12/17/19 9:36 PM, Al Viro wrote:
> On Fri, Dec 06, 2019 at 10:31:57AM -0600, Eric Sandeen wrote:
>> There doesn't seem to be a strong reason to have a copy of the
>> filesystem name string in the fs_parameter_description structure;
>> it's easy enough to get the name from the fs_type, and using it
>> instead ensures consistency across messages (for example,
>> vfs_parse_fs_param() already uses fc->fs_type->name for the error
>> messages, because it doesn't have the fs_parameter_description).
> 
> Arrgh...  That used to be fine.  Now we have this:
> static int rbd_parse_param(struct fs_parameter *param,
>                             struct rbd_parse_opts_ctx *pctx)
> {
>         struct rbd_options *opt = pctx->opts;
>         struct fs_parse_result result; 
>         int token, ret;
> 
>         ret = ceph_parse_param(param, pctx->copts, NULL);
>         if (ret != -ENOPARAM)
>                 return ret;
> 
>         token = fs_parse(NULL, rbd_parameters, param, &result);
> 			 ^^^^
> 
> 	Cthulhu damn it...  And yes, that crap used to work.
> Frankly, I'm tempted to allocate fs_context in there (in
> rbd_parse_options(), or in rbd_add_parse_args()) - we've other
> oddities due to that...
> 
> 	Alternatively, we could provide __fs_parse() that
> would take name as a separate argument and accepted NULL fc,
> with fs_parse() being a wrapper for that.
> 
> *grumble*

FYI be careful if you do munge this in, V2 inexplicably killed the name in
the fs_type for afs.  V3 fixed that thinko or whatever it was.

-Eric

Re: [PATCH V2] fs_parser: remove fs_parameter_description name field

[Al Viro]

On Tue, Dec 17, 2019 at 09:43:44PM -0600, Eric Sandeen wrote:
> On 12/17/19 9:36 PM, Al Viro wrote:
> > On Fri, Dec 06, 2019 at 10:31:57AM -0600, Eric Sandeen wrote:
> >> There doesn't seem to be a strong reason to have a copy of the
> >> filesystem name string in the fs_parameter_description structure;
> >> it's easy enough to get the name from the fs_type, and using it
> >> instead ensures consistency across messages (for example,
> >> vfs_parse_fs_param() already uses fc->fs_type->name for the error
> >> messages, because it doesn't have the fs_parameter_description).
> > 
> > Arrgh...  That used to be fine.  Now we have this:
> > static int rbd_parse_param(struct fs_parameter *param,
> >                             struct rbd_parse_opts_ctx *pctx)
> > {
> >         struct rbd_options *opt = pctx->opts;
> >         struct fs_parse_result result; 
> >         int token, ret;
> > 
> >         ret = ceph_parse_param(param, pctx->copts, NULL);
> >         if (ret != -ENOPARAM)
> >                 return ret;
> > 
> >         token = fs_parse(NULL, rbd_parameters, param, &result);
> > 			 ^^^^
> > 
> > 	Cthulhu damn it...  And yes, that crap used to work.
> > Frankly, I'm tempted to allocate fs_context in there (in
> > rbd_parse_options(), or in rbd_add_parse_args()) - we've other
> > oddities due to that...
> > 
> > 	Alternatively, we could provide __fs_parse() that
> > would take name as a separate argument and accepted NULL fc,
> > with fs_parse() being a wrapper for that.
> > 
> > *grumble*
> 
> FYI be careful if you do munge this in, V2 inexplicably killed the name in
> the fs_type for afs.  V3 fixed that thinko or whatever it was.

I used v3, anyway...  The reason I'm rather unhappy about the
entire situation is that in the end of that series I have
fs_param_is_u32() et.al. being _functions_.  With switch in
fs_parse() gone.

Typical instance looks like this:

int fs_param_is_enum(struct fs_context *fc, const struct fs_parameter_spec *p,
                     struct fs_parameter *param, struct fs_parse_result *result)
{
        const struct constant_table *c;
        if (param->type != fs_value_is_string)
                return fs_param_bad_value(fc, param);
        c = __lookup_constant(p->data, param->string);
        if (!c)
                return fs_param_bad_value(fc, param);
        result->uint_32 = c->value;
        return 0;
}

and I would rather not breed the arguments here ;-/  I could take logging
into the fs_parse() itself (it's very similar in all current instances),
but... if we go for something like

int fs_param_is_range(struct fs_context *fc, const struct fs_parameter_spec *p,
                     struct fs_parameter *param, struct fs_parse_result *result)
{
	const struct {u32 from, to;} *range = p->data;

        if (param->type != fs_value_is_string ||
	    kstrtouint(param->string, 0, &result->uint_32) < 0)
                return fs_param_bad_value(fc, param);
	
	if (result->uint_32 < range->from || result->uint_32 > range->to)
		return invalf(fc, "%s: Value for %s must be in [%u..%u]",
				fc->fs_type->name, param->key, range->from,
				range->to);
        return 0;
}
which is not all that unreasonable, the requirement of handling all
warnings in fs_parse() becomes unfeasible.  And the main reason for
conversion to method is the pressure to provide such custom types -
stuff like <number>{|K|M|G} for memory sizes, etc.

Shite...  We can, of course, pass the name to instances, but... *ugh*

Re: [PATCH V2] fs_parser: remove fs_parameter_description name field

[Al Viro]

On Wed, Dec 18, 2019 at 04:06:51AM +0000, Al Viro wrote:
> On Tue, Dec 17, 2019 at 09:43:44PM -0600, Eric Sandeen wrote:
> > On 12/17/19 9:36 PM, Al Viro wrote:
> > > On Fri, Dec 06, 2019 at 10:31:57AM -0600, Eric Sandeen wrote:
> > >> There doesn't seem to be a strong reason to have a copy of the
> > >> filesystem name string in the fs_parameter_description structure;
> > >> it's easy enough to get the name from the fs_type, and using it
> > >> instead ensures consistency across messages (for example,
> > >> vfs_parse_fs_param() already uses fc->fs_type->name for the error
> > >> messages, because it doesn't have the fs_parameter_description).
> > > 
> > > Arrgh...  That used to be fine.  Now we have this:
> > > static int rbd_parse_param(struct fs_parameter *param,
> > >                             struct rbd_parse_opts_ctx *pctx)
> > > {
> > >         struct rbd_options *opt = pctx->opts;
> > >         struct fs_parse_result result; 
> > >         int token, ret;
> > > 
> > >         ret = ceph_parse_param(param, pctx->copts, NULL);
> > >         if (ret != -ENOPARAM)
> > >                 return ret;
> > > 
> > >         token = fs_parse(NULL, rbd_parameters, param, &result);
> > > 			 ^^^^
> > > 
> > > 	Cthulhu damn it...  And yes, that crap used to work.
> > > Frankly, I'm tempted to allocate fs_context in there (in
> > > rbd_parse_options(), or in rbd_add_parse_args()) - we've other
> > > oddities due to that...
> > > 
> > > 	Alternatively, we could provide __fs_parse() that
> > > would take name as a separate argument and accepted NULL fc,
> > > with fs_parse() being a wrapper for that.
> > > 
> > > *grumble*
> > 
> > FYI be careful if you do munge this in, V2 inexplicably killed the name in
> > the fs_type for afs.  V3 fixed that thinko or whatever it was.
> 
> I used v3, anyway...  The reason I'm rather unhappy about the
> entire situation is that in the end of that series I have
> fs_param_is_u32() et.al. being _functions_.  With switch in
> fs_parse() gone.
> 
> Typical instance looks like this:
> 
> int fs_param_is_enum(struct fs_context *fc, const struct fs_parameter_spec *p,
>                      struct fs_parameter *param, struct fs_parse_result *result)
> {
>         const struct constant_table *c;
>         if (param->type != fs_value_is_string)
>                 return fs_param_bad_value(fc, param);
>         c = __lookup_constant(p->data, param->string);
>         if (!c)
>                 return fs_param_bad_value(fc, param);
>         result->uint_32 = c->value;
>         return 0;
> }
> 
> and I would rather not breed the arguments here ;-/  I could take logging
> into the fs_parse() itself (it's very similar in all current instances),
> but... if we go for something like
> 
> int fs_param_is_range(struct fs_context *fc, const struct fs_parameter_spec *p,
>                      struct fs_parameter *param, struct fs_parse_result *result)
> {
> 	const struct {u32 from, to;} *range = p->data;
> 
>         if (param->type != fs_value_is_string ||
> 	    kstrtouint(param->string, 0, &result->uint_32) < 0)
>                 return fs_param_bad_value(fc, param);
> 	
> 	if (result->uint_32 < range->from || result->uint_32 > range->to)
> 		return invalf(fc, "%s: Value for %s must be in [%u..%u]",
> 				fc->fs_type->name, param->key, range->from,
> 				range->to);
>         return 0;
> }
> which is not all that unreasonable, the requirement of handling all
> warnings in fs_parse() becomes unfeasible.  And the main reason for
> conversion to method is the pressure to provide such custom types -
> stuff like <number>{|K|M|G} for memory sizes, etc.
> 
> Shite...  We can, of course, pass the name to instances, but... *ugh*

OK...  Some observations after looking through that stuff:

1) there is a legitimate need to use fs_parse() (or at least its underlying
machinery) outside of anything mount-related.  One case already in mainline
is ceph and rbd sharing parts of the syntax (libceph stuff).  There almost
certainly will be more.

2) fs_parse() is a bit of misnomer - it decides which option it is and
does things like conversion of string to number, etc., but it does not
manipulate the parser state.  Its _caller_ (->parse_param(), usually)
does.

3) right now the only part of fs_context used by the damn thing is fc->log;
Eric's patch adds fc->fs_type->name.  Note that it's used only for logging
purposes.

4) the primitives used for logging (invalf, errorf, warnf) are taking
fs_context; however, the only part being used is fc->log.  NULL fs_context
is treated as NULL fc_log, which means "log via printk".

5) absolute majority of log messages are prefixed, usually by fs type
name.  The few that are not probably ought to be - e.g. the things like
arch/powerpc/platforms/cell/spufs/inode.c:629:                  return invalf(fc, "Unknown uid");
would be better off with spufs: unknown uid.

We could require _some_ fs_context to be supplied by all callers,
mount-related or not.  However, considering the above, that looks rather
unnatural, especially if we go ahead and allow fc->fs_type->name uses.

Alternatively, we could pass fc_log + prefix; fs_parse() itself would
remain as-is, becoming a wrapper for __fs_parse() that would take
fs_log + string instead of fs_context.  Potentially unpleasant part
that way: option recognition has no access to the data from previously
processed options.  Another inconvenience is that we get an extra
argument to pass into fs_param_is_...().

One note on logfc(): it tries to be smarter than it's worth being -
in the reality it _never_ gets bare "%s" as format; there's always
"w ", "e " or "i " in the beginning.  So this part
        if (strcmp(fmt, "%s") == 0) {
                p = va_arg(va, const char *);
                goto unformatted_string;
        }
in there is pointless (at least in the current form).

I wonder if we should do the following:
	* structure with two members - pointer to fc_log and a string
(prefix) embedded into fs_context, in place of ->log.
	* __logfc() taking pointer to that struct, integer or
character representing the "log level", then format and vararg part.
	* warnf() being simply __logfc(&fc->log, 'w', fmt, ## __VA_ARGS__)
	* __logfc() using "%c %s%s%pV",
				loglevel,
				prefix?prefix:"",
				prefix ? ":" : "",
				fmt, va
for kvasprintf() (assuming that %pV *can* be used with it, of course)
	* const char *set_log_prefix(pointer, string) replacing the
prefix field of the struct and returning the original.  fs_context
allocation would set it to fs_type->name.
	* __fs_parse() would be taking a pointer to that field of
fs_context instead of the entire thing; ditto for fs_param_is_...()
	* rbd would create a local structure with "rbd" for prefix
and NULL for log
	* net/ceph would replace the prefix in the thing it has
been given with "libceph" and revert back to original in the end

The most worrying part in that is kvasprintf interplay with %pV -
we might need to open-code it, since we need va_copy() not of that
sucker's arguments, but of the va_list one level deeper.  But with
that open-coding (and it's not all that scary, really -
	va_list va;
	va_list va_copy;

	va_start(va, fmt);
	va_copy(va_copy, va);
	size = snprintf(NULL, 0, "%c %s%s%pV",
			loglevel, prefix ? prefix : "",
			prefix ? ":" : "", fmt, va_copy);
	va_end(va_copy);
	p = kmalloc(size + 1, GFP_KERNEL);
	if (!p)
		sod off
	snprintf(p, size + 1, "%c %s%s%pV",
		loglevel, prefix?prefix:"",
		prefix ? ":" : "", fmt, va);
	va_end(va);
) it ought to be feasible, AFAICS...

Comments?  Oh, and for "log to printk" case we can bloody well
get rid of allocation and just do printk with the right
loglevel and format modified in the obvious way...

Re: [PATCH V2] fs_parser: remove fs_parameter_description name field

[Al Viro]

On Thu, Dec 19, 2019 at 11:29:51PM +0000, Al Viro wrote:

> I wonder if we should do the following:
> 	* structure with two members - pointer to fc_log and a string
> (prefix) embedded into fs_context, in place of ->log.
> 	* __logfc() taking pointer to that struct, integer or
> character representing the "log level", then format and vararg part.
> 	* warnf() being simply __logfc(&fc->log, 'w', fmt, ## __VA_ARGS__)
> 	* __logfc() using "%c %s%s%pV",
> 				loglevel,
> 				prefix?prefix:"",
> 				prefix ? ":" : "",
> 				fmt, va
> for kvasprintf() (assuming that %pV *can* be used with it, of course)
> 	* const char *set_log_prefix(pointer, string) replacing the
> prefix field of the struct and returning the original.  fs_context
> allocation would set it to fs_type->name.
> 	* __fs_parse() would be taking a pointer to that field of
> fs_context instead of the entire thing; ditto for fs_param_is_...()
> 	* rbd would create a local structure with "rbd" for prefix
> and NULL for log
> 	* net/ceph would replace the prefix in the thing it has
> been given with "libceph" and revert back to original in the end
> 
> The most worrying part in that is kvasprintf interplay with %pV -
> we might need to open-code it, since we need va_copy() not of that
> sucker's arguments, but of the va_list one level deeper.

We won't - va_format() itself does take a copy.  So no open-coding
is needed, kasprint() would work.  OK, that simplifies things...