diff mbox series

[RFC,4/8] vfs: Convert pstore to fs_context

Message ID 155316888615.29437.17558157678296689830.stgit@warthog.procyon.org.uk (mailing list archive)
State New, archived
Headers show
Series Convert mount_single-using filesystems to fs_context | expand

Commit Message

David Howells March 21, 2019, 11:48 a.m. UTC 
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Kees Cook <keescook@chromium.org>
cc: Anton Vorontsov <anton@enomsg.org>
cc: Colin Cross <ccross@android.com>
cc: Tony Luck <tony.luck@intel.com>
---

 fs/pstore/inode.c |  110 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 39 deletions(-)

Comments

Kees Cook March 21, 2019, 4:31 p.m. UTC | #1 
On Thu, Mar 21, 2019 at 4:48 AM David Howells <dhowells@redhat.com> wrote:
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Kees Cook <keescook@chromium.org>
> cc: Anton Vorontsov <anton@enomsg.org>
> cc: Colin Cross <ccross@android.com>
> cc: Tony Luck <tony.luck@intel.com>

Thanks for doing this conversion!

Acked-by: Kees Cook <keescook@chromium.org>

Some questions/nits below:

> +static int pstore_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +       struct pstore_fs_context *ctx = fc->fs_private;
> +       struct fs_parse_result result;
> +       int opt;
> +
> +       opt = fs_parse(fc, &pstore_fs_parameters, param, &result);
> +       if (opt < 0)
> +               return opt;
> +
> +       switch (opt) {
> +       case Opt_kmsg_bytes:
> +               ctx->kmsg_bytes = result.uint_32;
> +               break;
> +       }
>
> -       while ((p = strsep(&options, ",")) != NULL) {
> -               int token;
> +       return 0;
> +}
>
> -               if (!*p)
> -                       continue;
> +static void pstore_apply_param(struct fs_context *fc)
> +{
> +       struct pstore_fs_context *ctx = fc->fs_private;
>
> -               token = match_token(p, tokens, args);
> -               switch (token) {
> -               case Opt_kmsg_bytes:
> -                       if (!match_int(&args[0], &option))
> -                               pstore_set_kmsg_bytes(option);
> -                       break;
> -               }
> -       }
> +       pstore_set_kmsg_bytes(ctx->kmsg_bytes);
>  }

Why the separation between parse and apply now? Is this due to the
reconfigure calls? (i.e. why not call pstore_set_kmsg_bytes() in
pstore_parse_param()?

> @@ -416,14 +423,38 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent)
>                 return -ENOMEM;
>
>         pstore_get_records(0);
> -
>         return 0;
>  }

I prefer to keep a blank before "return", but no need to drop this
unless it gets a respin.

Thanks again!
David Howells March 21, 2019, 5:03 p.m. UTC | #2 
Kees Cook <keescook@chromium.org> wrote:

> Why the separation between parse and apply now? Is this due to the
> reconfigure calls? (i.e. why not call pstore_set_kmsg_bytes() in
> pstore_parse_param()?

Because parameter parsing is now done up front, before the creation of the
superblock of the invocation of the reconfigure method - so there's still a
bunch of places that can error out before you know you're going to be
successful in creating/reconfiguring the superblock.

David
Kees Cook March 21, 2019, 5:35 p.m. UTC | #3 
On Thu, Mar 21, 2019 at 10:03 AM David Howells <dhowells@redhat.com> wrote:
>
> Kees Cook <keescook@chromium.org> wrote:
>
> > Why the separation between parse and apply now? Is this due to the
> > reconfigure calls? (i.e. why not call pstore_set_kmsg_bytes() in
> > pstore_parse_param()?
>
> Because parameter parsing is now done up front, before the creation of the
> superblock of the invocation of the reconfigure method - so there's still a
> bunch of places that can error out before you know you're going to be
> successful in creating/reconfiguring the superblock.

Thanks! Makes sense. :)
diff mbox series

Patch

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index c60ee46f3e39..4625246fa992 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -19,6 +19,8 @@ 
 
 #include <linux/module.h>
 #include <linux/fs.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/fsnotify.h>
 #include <linux/pagemap.h>
 #include <linux/highmem.h>
@@ -26,10 +28,8 @@ 
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/string.h>
-#include <linux/mount.h>
 #include <linux/seq_file.h>
 #include <linux/ramfs.h>
-#include <linux/parser.h>
 #include <linux/sched.h>
 #include <linux/magic.h>
 #include <linux/pstore.h>
@@ -227,38 +227,48 @@  static struct inode *pstore_get_inode(struct super_block *sb)
 	return inode;
 }
 
+struct pstore_fs_context {
+	unsigned int	kmsg_bytes;
+};
+
 enum {
-	Opt_kmsg_bytes, Opt_err
+	Opt_kmsg_bytes,
 };
 
-static const match_table_t tokens = {
-	{Opt_kmsg_bytes, "kmsg_bytes=%u"},
-	{Opt_err, NULL}
+static const struct fs_parameter_spec pstore_param_specs[] = {
+	fsparam_u32	("kmsg_bytes",		Opt_kmsg_bytes),
+	{}
 };
 
-static void parse_options(char *options)
-{
-	char		*p;
-	substring_t	args[MAX_OPT_ARGS];
-	int		option;
+static const struct fs_parameter_description pstore_fs_parameters = {
+	.name		= "pstore",
+	.specs		= pstore_param_specs,
+};
 
-	if (!options)
-		return;
+static int pstore_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+	struct pstore_fs_context *ctx = fc->fs_private;
+	struct fs_parse_result result;
+	int opt;
+
+	opt = fs_parse(fc, &pstore_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_kmsg_bytes:
+		ctx->kmsg_bytes = result.uint_32;
+		break;
+	}
 
-	while ((p = strsep(&options, ",")) != NULL) {
-		int token;
+	return 0;
+}
 
-		if (!*p)
-			continue;
+static void pstore_apply_param(struct fs_context *fc)
+{
+	struct pstore_fs_context *ctx = fc->fs_private;
 
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_kmsg_bytes:
-			if (!match_int(&args[0], &option))
-				pstore_set_kmsg_bytes(option);
-			break;
-		}
-	}
+	pstore_set_kmsg_bytes(ctx->kmsg_bytes);
 }
 
 /*
@@ -271,11 +281,10 @@  static int pstore_show_options(struct seq_file *m, struct dentry *root)
 	return 0;
 }
 
-static int pstore_remount(struct super_block *sb, int *flags, char *data)
+static int pstore_reconfigure(struct fs_context *fc)
 {
-	sync_filesystem(sb);
-	parse_options(data);
-
+	sync_filesystem(fc->root->d_sb);
+	pstore_apply_param(fc);
 	return 0;
 }
 
@@ -283,7 +292,6 @@  static const struct super_operations pstore_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
 	.evict_inode	= pstore_evict_inode,
-	.remount_fs	= pstore_remount,
 	.show_options	= pstore_show_options,
 };
 
@@ -389,12 +397,10 @@  void pstore_get_records(int quiet)
 	inode_unlock(d_inode(root));
 }
 
-static int pstore_fill_super(struct super_block *sb, void *data, int silent)
+static int pstore_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct inode *inode;
 
-	pstore_sb = sb;
-
 	sb->s_maxbytes		= MAX_LFS_FILESIZE;
 	sb->s_blocksize		= PAGE_SIZE;
 	sb->s_blocksize_bits	= PAGE_SHIFT;
@@ -402,7 +408,8 @@  static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op		= &pstore_ops;
 	sb->s_time_gran		= 1;
 
-	parse_options(data);
+	pstore_sb = sb;
+	pstore_apply_param(fc);
 
 	inode = pstore_get_inode(sb);
 	if (inode) {
@@ -416,14 +423,38 @@  static int pstore_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 
 	pstore_get_records(0);
-
 	return 0;
 }
 
-static struct dentry *pstore_mount(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data)
+static int pstore_get_tree(struct fs_context *fc)
+{
+	return vfs_get_super(fc, vfs_get_single_reconf_super,
+			     pstore_fill_super);
+}
+
+static void pstore_free_fc(struct fs_context *fc)
 {
-	return mount_single(fs_type, flags, data, pstore_fill_super);
+	kfree(fc->fs_private);
+}
+
+static const struct fs_context_operations pstore_context_ops = {
+	.free		= pstore_free_fc,
+	.parse_param	= pstore_parse_param,
+	.get_tree	= pstore_get_tree,
+	.reconfigure	= pstore_reconfigure,
+};
+
+static int pstore_init_fs_context(struct fs_context *fc)
+{
+	struct pstore_fs_context *ctx;
+
+	ctx = kzalloc(sizeof(struct pstore_fs_context), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	fc->fs_private = ctx;
+	fc->ops = &pstore_context_ops;
+	return 0;
 }
 
 static void pstore_kill_sb(struct super_block *sb)
@@ -435,7 +466,8 @@  static void pstore_kill_sb(struct super_block *sb)
 static struct file_system_type pstore_fs_type = {
 	.owner          = THIS_MODULE,
 	.name		= "pstore",
-	.mount		= pstore_mount,
+	.init_fs_context = pstore_init_fs_context,
+	.parameters	= &pstore_fs_parameters,
 	.kill_sb	= pstore_kill_sb,
 };