[PATCH v2 1/8] seq_file: introduce seq_open_data helper

[PATCH v2 1/8] seq_file: introduce seq_open_data helper

[Rasmus Villemoes]

There are quite a few callers of seq_open that could be simplified by
setting the ->private member via the seq_open call instead of fetching
file->private_data afterwards.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2:
- Fix some copy-pastos spotted by Andreas.
- Ensure everybody hit by an example patch also gets this cover letter/introducing patch.
- Include a few fs/ examples.

I've just included a few examples of possible users of this helper,
there are many more similar cases. As a bonus, 7/8 fix a potential
NULL deref (if one believes that seq_open can actually fail).

seq_open_private would have been a better name, but that one is
already taken...

Documentation/filesystems/seq_file.txt |  9 +++++----
 fs/seq_file.c                          | 16 ++++++++++++----
 include/linux/seq_file.h               |  1 +
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
index 9de4303201e1..68571b8275d8 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -234,10 +234,11 @@ Here, the call to seq_open() takes the seq_operations structure we created
 before, and gets set up to iterate through the virtual file.
 
 On a successful open, seq_open() stores the struct seq_file pointer in
-file->private_data. If you have an application where the same iterator can
-be used for more than one file, you can store an arbitrary pointer in the
-private field of the seq_file structure; that value can then be retrieved
-by the iterator functions.
+file->private_data. If you have an application where the same iterator
+can be used for more than one file, you can store an arbitrary pointer
+in the private field of the seq_file structure; that value can then be
+retrieved by the iterator functions. Using the wrapper seq_open_data()
+allows you to set the initial value for that field.
 
 There is also a wrapper function to seq_open() called seq_open_private(). It
 kmallocs a zero filled block of memory and stores a pointer to it in the
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 4cc090b50cc5..c8c86660f6db 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -33,11 +33,12 @@ static void *seq_buf_alloc(unsigned long size)
 }
 
 /**
- *	seq_open -	initialize sequential file
+ *	seq_open_data -	initialize sequential file
  *	@file: file we initialize
  *	@op: method table describing the sequence
+ *	@data: initial value for ->private field
  *
- *	seq_open() sets @file, associating it with a sequence described
+ *	seq_open_data() sets @file, associating it with a sequence described
  *	by @op.  @op->start() sets the iterator up and returns the first
  *	element of sequence. @op->stop() shuts it down.  @op->next()
  *	returns the next element of sequence.  @op->show() prints element
@@ -45,10 +46,10 @@ static void *seq_buf_alloc(unsigned long size)
  *	ERR_PTR(error).  In the end of sequence they return %NULL. ->show()
  *	returns 0 in case of success and negative number in case of error.
  *	Returning SEQ_SKIP means "discard this element and move on".
- *	Note: seq_open() will allocate a struct seq_file and store its
+ *	Note: seq_open_data() will allocate a struct seq_file and store its
  *	pointer in @file->private_data. This pointer should not be modified.
  */
-int seq_open(struct file *file, const struct seq_operations *op)
+int seq_open_data(struct file *file, const struct seq_operations *op, void *data)
 {
 	struct seq_file *p;
 
@@ -62,6 +63,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
 
 	mutex_init(&p->lock);
 	p->op = op;
+	p->private = data;
 
 	// No refcounting: the lifetime of 'p' is constrained
 	// to the lifetime of the file.
@@ -86,6 +88,12 @@ int seq_open(struct file *file, const struct seq_operations *op)
 	file->f_mode &= ~FMODE_PWRITE;
 	return 0;
 }
+EXPORT_SYMBOL(seq_open_data);
+
+int seq_open(struct file *file, const struct seq_operations *op)
+{
+	return seq_open_data(file, op, NULL);
+}
 EXPORT_SYMBOL(seq_open);
 
 static int traverse(struct seq_file *m, loff_t offset)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index a121982af0f5..1142e39bfad2 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,7 @@ void seq_pad(struct seq_file *m, char c);
 
 char *mangle_path(char *s, const char *p, const char *esc);
 int seq_open(struct file *, const struct seq_operations *);
+int seq_open_data(struct file *, const struct seq_operations *, void *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
 loff_t seq_lseek(struct file *, loff_t, int);
 int seq_release(struct inode *, struct file *);
-- 
2.16.4

[PATCH v2 7/8] powerpc/pseries: use seq_open_data in hcall_inst_seq_open

[Rasmus Villemoes]

This code should check the return value of seq_open(); if it failed,
file->private_data is NULL. But we can avoid the issue entirely and
simplify the code by letting seq_open_data() set the ->private member.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Depends on 1/8 introducing seq_open_data.

arch/powerpc/platforms/pseries/hvCall_inst.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hvCall_inst.c b/arch/powerpc/platforms/pseries/hvCall_inst.c
index 6da320c786cd..4816916dc8a1 100644
--- a/arch/powerpc/platforms/pseries/hvCall_inst.c
+++ b/arch/powerpc/platforms/pseries/hvCall_inst.c
@@ -92,14 +92,7 @@ static const struct seq_operations hcall_inst_seq_ops = {
 
 static int hcall_inst_seq_open(struct inode *inode, struct file *file)
 {
-	int rc;
-	struct seq_file *seq;
-
-	rc = seq_open(file, &hcall_inst_seq_ops);
-	seq = file->private_data;
-	seq->private = file_inode(file)->i_private;
-
-	return rc;
+	return seq_open_data(file, &hcall_inst_seq_ops, file_inode(file)->i_private);
 }
 
 static const struct file_operations hcall_inst_seq_fops = {
-- 
2.16.4

Re: [PATCH v2 1/8] seq_file: introduce seq_open_data helper

[Andy Shevchenko]

On Sat, Aug 18, 2018 at 4:26 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> There are quite a few callers of seq_open that could be simplified by
> setting the ->private member via the seq_open call instead of fetching
> file->private_data afterwards.
>

I like this series,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

P.S. ...though it seems patch 3 missed a commit message.

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> v2:
> - Fix some copy-pastos spotted by Andreas.
> - Ensure everybody hit by an example patch also gets this cover letter/introducing patch.
> - Include a few fs/ examples.
>
> I've just included a few examples of possible users of this helper,
> there are many more similar cases. As a bonus, 7/8 fix a potential
> NULL deref (if one believes that seq_open can actually fail).
>
> seq_open_private would have been a better name, but that one is
> already taken...
>
> Documentation/filesystems/seq_file.txt |  9 +++++----
>  fs/seq_file.c                          | 16 ++++++++++++----
>  include/linux/seq_file.h               |  1 +
>  3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index 9de4303201e1..68571b8275d8 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -234,10 +234,11 @@ Here, the call to seq_open() takes the seq_operations structure we created
>  before, and gets set up to iterate through the virtual file.
>
>  On a successful open, seq_open() stores the struct seq_file pointer in
> -file->private_data. If you have an application where the same iterator can
> -be used for more than one file, you can store an arbitrary pointer in the
> -private field of the seq_file structure; that value can then be retrieved
> -by the iterator functions.
> +file->private_data. If you have an application where the same iterator
> +can be used for more than one file, you can store an arbitrary pointer
> +in the private field of the seq_file structure; that value can then be
> +retrieved by the iterator functions. Using the wrapper seq_open_data()
> +allows you to set the initial value for that field.
>
>  There is also a wrapper function to seq_open() called seq_open_private(). It
>  kmallocs a zero filled block of memory and stores a pointer to it in the
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 4cc090b50cc5..c8c86660f6db 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -33,11 +33,12 @@ static void *seq_buf_alloc(unsigned long size)
>  }
>
>  /**
> - *     seq_open -      initialize sequential file
> + *     seq_open_data - initialize sequential file
>   *     @file: file we initialize
>   *     @op: method table describing the sequence
> + *     @data: initial value for ->private field
>   *
> - *     seq_open() sets @file, associating it with a sequence described
> + *     seq_open_data() sets @file, associating it with a sequence described
>   *     by @op.  @op->start() sets the iterator up and returns the first
>   *     element of sequence. @op->stop() shuts it down.  @op->next()
>   *     returns the next element of sequence.  @op->show() prints element
> @@ -45,10 +46,10 @@ static void *seq_buf_alloc(unsigned long size)
>   *     ERR_PTR(error).  In the end of sequence they return %NULL. ->show()
>   *     returns 0 in case of success and negative number in case of error.
>   *     Returning SEQ_SKIP means "discard this element and move on".
> - *     Note: seq_open() will allocate a struct seq_file and store its
> + *     Note: seq_open_data() will allocate a struct seq_file and store its
>   *     pointer in @file->private_data. This pointer should not be modified.
>   */
> -int seq_open(struct file *file, const struct seq_operations *op)
> +int seq_open_data(struct file *file, const struct seq_operations *op, void *data)
>  {
>         struct seq_file *p;
>
> @@ -62,6 +63,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
>
>         mutex_init(&p->lock);
>         p->op = op;
> +       p->private = data;
>
>         // No refcounting: the lifetime of 'p' is constrained
>         // to the lifetime of the file.
> @@ -86,6 +88,12 @@ int seq_open(struct file *file, const struct seq_operations *op)
>         file->f_mode &= ~FMODE_PWRITE;
>         return 0;
>  }
> +EXPORT_SYMBOL(seq_open_data);
> +
> +int seq_open(struct file *file, const struct seq_operations *op)
> +{
> +       return seq_open_data(file, op, NULL);
> +}
>  EXPORT_SYMBOL(seq_open);
>
>  static int traverse(struct seq_file *m, loff_t offset)
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index a121982af0f5..1142e39bfad2 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -107,6 +107,7 @@ void seq_pad(struct seq_file *m, char c);
>
>  char *mangle_path(char *s, const char *p, const char *esc);
>  int seq_open(struct file *, const struct seq_operations *);
> +int seq_open_data(struct file *, const struct seq_operations *, void *);
>  ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
>  loff_t seq_lseek(struct file *, loff_t, int);
>  int seq_release(struct inode *, struct file *);
> --
> 2.16.4
>


-- 
With Best Regards,
Andy Shevchenko