* [PATCH v2 1/8] seq_file: introduce seq_open_data helper
@ 2018-08-18 13:24 Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 2/8] seq_file: use seq_open_data in single_open Rasmus Villemoes
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2018-08-18 13:24 UTC (permalink / raw)
To: Jonathan Corbet, Alexander Viro
Cc: linux-kernel, Rasmus Villemoes, Andreas Dilger, linux-doc,
linux-fsdevel, Alexey Dobriyan, linux-nfs, cluster-devel,
Michael Ellerman, linuxppc-dev, Thierry Reding,
Lorenzo Pieralisi
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/8] seq_file: use seq_open_data in single_open
2018-08-18 13:24 [PATCH v2 1/8] seq_file: introduce seq_open_data helper Rasmus Villemoes
@ 2018-08-18 13:24 ` Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 3/8] seq_file: use seq_open_data in __seq_open_private Rasmus Villemoes
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2018-08-18 13:24 UTC (permalink / raw)
To: Alexander Viro; +Cc: linux-kernel, Rasmus Villemoes, linux-fsdevel
Avoid the somewhat hard to grok assignment by using the seq_open_data
helper.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
fs/seq_file.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index c8c86660f6db..518a72e444d9 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -582,10 +582,8 @@ int single_open(struct file *file, int (*show)(struct seq_file *, void *),
op->next = single_next;
op->stop = single_stop;
op->show = show;
- res = seq_open(file, op);
- if (!res)
- ((struct seq_file *)file->private_data)->private = data;
- else
+ res = seq_open_data(file, op, data);
+ if (res)
kfree(op);
}
return res;
--
2.16.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 3/8] seq_file: use seq_open_data in __seq_open_private
2018-08-18 13:24 [PATCH v2 1/8] seq_file: introduce seq_open_data helper Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 2/8] seq_file: use seq_open_data in single_open Rasmus Villemoes
@ 2018-08-18 13:24 ` Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 4/8] proc: use seq_open_data() in proc_id_map_open() Rasmus Villemoes
2018-08-19 12:21 ` [PATCH v2 1/8] seq_file: introduce seq_open_data helper Andy Shevchenko
3 siblings, 0 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2018-08-18 13:24 UTC (permalink / raw)
To: Alexander Viro; +Cc: linux-kernel, Rasmus Villemoes, linux-fsdevel
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
fs/seq_file.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 518a72e444d9..5cc4670294e7 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -632,18 +632,15 @@ void *__seq_open_private(struct file *f, const struct seq_operations *ops,
{
int rc;
void *private;
- struct seq_file *seq;
private = kzalloc(psize, GFP_KERNEL_ACCOUNT);
if (private == NULL)
goto out;
- rc = seq_open(f, ops);
+ rc = seq_open_data(f, ops, private);
if (rc < 0)
goto out_free;
- seq = f->private_data;
- seq->private = private;
return private;
out_free:
--
2.16.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 4/8] proc: use seq_open_data() in proc_id_map_open()
2018-08-18 13:24 [PATCH v2 1/8] seq_file: introduce seq_open_data helper Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 2/8] seq_file: use seq_open_data in single_open Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 3/8] seq_file: use seq_open_data in __seq_open_private Rasmus Villemoes
@ 2018-08-18 13:24 ` Rasmus Villemoes
2018-08-19 12:21 ` [PATCH v2 1/8] seq_file: introduce seq_open_data helper Andy Shevchenko
3 siblings, 0 replies; 5+ messages in thread
From: Rasmus Villemoes @ 2018-08-18 13:24 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: linux-kernel, Rasmus Villemoes, linux-fsdevel
Simplify the code slightly by using the seq_open_data helper.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Depends on 1/8 introducing seq_open_data.
fs/proc/base.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index aaffc0c30216..32ed72c44412 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2747,7 +2747,6 @@ static int proc_id_map_open(struct inode *inode, struct file *file,
{
struct user_namespace *ns = NULL;
struct task_struct *task;
- struct seq_file *seq;
int ret = -EINVAL;
task = get_proc_task(inode);
@@ -2758,19 +2757,11 @@ static int proc_id_map_open(struct inode *inode, struct file *file,
put_task_struct(task);
}
if (!ns)
- goto err;
+ return ret;
- ret = seq_open(file, seq_ops);
+ ret = seq_open_data(file, seq_ops, ns);
if (ret)
- goto err_put_ns;
-
- seq = file->private_data;
- seq->private = ns;
-
- return 0;
-err_put_ns:
- put_user_ns(ns);
-err:
+ put_user_ns(ns);
return ret;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/8] seq_file: introduce seq_open_data helper
2018-08-18 13:24 [PATCH v2 1/8] seq_file: introduce seq_open_data helper Rasmus Villemoes
` (2 preceding siblings ...)
2018-08-18 13:24 ` [PATCH v2 4/8] proc: use seq_open_data() in proc_id_map_open() Rasmus Villemoes
@ 2018-08-19 12:21 ` Andy Shevchenko
3 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2018-08-19 12:21 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Jonathan Corbet, Alexander Viro, Linux Kernel Mailing List,
adilger, Linux Documentation List, Linux FS Devel,
Alexey Dobriyan, linux-nfs, cluster-devel, Michael Ellerman,
open list:LINUX FOR POWERPC PA SEMI PWRFICIENT, Thierry Reding,
Lorenzo Pieralisi
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-19 12:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-18 13:24 [PATCH v2 1/8] seq_file: introduce seq_open_data helper Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 2/8] seq_file: use seq_open_data in single_open Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 3/8] seq_file: use seq_open_data in __seq_open_private Rasmus Villemoes
2018-08-18 13:24 ` [PATCH v2 4/8] proc: use seq_open_data() in proc_id_map_open() Rasmus Villemoes
2018-08-19 12:21 ` [PATCH v2 1/8] seq_file: introduce seq_open_data helper Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).