[PATCH 0/2] fs: proc: use seq_open_private()

[PATCH 0/2] fs: proc: use seq_open_private()

[Rob Jones]

Use __seq_open_private() to reduce boilerplate code in proc.

This function has been around, undocumented, for years. It can simplify
the set up code for seq file operations.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>


Rob Jones (2):
  fs: proc: use __seq_open_private()
  fs: proc: use __seq_open_private()

 fs/proc/task_mmu.c   |   42 ++++++++++++++++--------------------------
 fs/proc/task_nommu.c |   22 ++++++++--------------
 2 files changed, 24 insertions(+), 40 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] fs: proc: use __seq_open_private()

[Rob Jones]

Reduce boilerplate code by using __seq_open_private() instead of seq_open().

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 fs/proc/task_nommu.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 678455d..b141050 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -269,20 +269,14 @@ static int maps_open(struct inode *inode, struct file *file,
 		     const struct seq_operations *ops)
 {
 	struct proc_maps_private *priv;
-	int ret = -ENOMEM;
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (priv) {
-		priv->pid = proc_pid(inode);
-		ret = seq_open(file, ops);
-		if (!ret) {
-			struct seq_file *m = file->private_data;
-			m->private = priv;
-		} else {
-			kfree(priv);
-		}
-	}
-	return ret;
+
+	priv = __seq_open_private(file, ops, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pid = proc_pid(inode);
+
+	return 0;
 }
 
 static int pid_maps_open(struct inode *inode, struct file *file)
-- 
1.7.10.4

[PATCH 2/2] fs: proc: use __seq_open_private()

[Rob Jones]

Reduce boilerplate code by using __seq_open_private() instead of seq_open().

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 fs/proc/task_mmu.c |   42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 442177b..c8e9045 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -235,19 +235,14 @@ static int do_maps_open(struct inode *inode, struct file *file,
 			const struct seq_operations *ops)
 {
 	struct proc_maps_private *priv;
-	int ret = -ENOMEM;
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (priv) {
-		priv->pid = proc_pid(inode);
-		ret = seq_open(file, ops);
-		if (!ret) {
-			struct seq_file *m = file->private_data;
-			m->private = priv;
-		} else {
-			kfree(priv);
-		}
-	}
-	return ret;
+
+	priv = __seq_open_private(file, ops, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pid = proc_pid(inode);
+
+	return 0;
 }
 
 static void
@@ -1495,19 +1490,14 @@ static int numa_maps_open(struct inode *inode, struct file *file,
 			  const struct seq_operations *ops)
 {
 	struct numa_maps_private *priv;
-	int ret = -ENOMEM;
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (priv) {
-		priv->proc_maps.pid = proc_pid(inode);
-		ret = seq_open(file, ops);
-		if (!ret) {
-			struct seq_file *m = file->private_data;
-			m->private = priv;
-		} else {
-			kfree(priv);
-		}
-	}
-	return ret;
+
+	priv = __seq_open_private(file, ops, sizeof(*priv));
+	if (!priv)
+		return -ENOMEM;
+
+	priv->proc_maps.pid = proc_pid(inode);
+
+	return 0;
 }
 
 static int pid_numa_maps_open(struct inode *inode, struct file *file)
-- 
1.7.10.4

Re: [PATCH 0/2] fs: proc: use seq_open_private()

[Andrew Morton]

On Fri, 12 Sep 2014 15:09:36 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

>   fs: proc: use __seq_open_private()
>   fs: proc: use __seq_open_private()

See the problem?  We have two different patches, both named the same.

I renamed them to

	fs/proc/task_nommu.c: use __seq_open_private()
	fs/proc/task_mmu.c: use __seq_open_private()

I really don't understand this practice of replacing "/" with ": " in
patch titles.  Why not just use the "/"?

Re: [PATCH 1/2] fs: proc: use __seq_open_private()

[Andrew Morton]

On Fri, 12 Sep 2014 15:09:37 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

> Reduce boilerplate code by using __seq_open_private() instead of seq_open().

http://ozlabs.org/~akpm/mmots/broken-out/fs-proc-task_nommuc-change-maps_open-to-use-__seq_open_private.patch
already did this.

Re: [PATCH 0/2] fs: proc: use seq_open_private()

[Rob Jones]

On 12/09/14 22:50, Andrew Morton wrote:
> On Fri, 12 Sep 2014 15:09:36 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>>    fs: proc: use __seq_open_private()
>>    fs: proc: use __seq_open_private()
>
> See the problem?  We have two different patches, both named the same.

Always another gotcha! :-)

Seriously, does it say anywhere that patch names have to be unique? It
makes perfect sense when it's pointed out but it never occurred to me.

I'll make sure I don't do it again.

>
> I renamed them to
>
> 	fs/proc/task_nommu.c: use __seq_open_private()
> 	fs/proc/task_mmu.c: use __seq_open_private()

Thank you, much appreciated. I would have been happy to re-submit.

>
> I really don't understand this practice of replacing "/" with ": " in
> patch titles.  Why not just use the "/"?

I'll do this in future too.

Sigh. So much to learn.

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH 0/2] fs: proc: use seq_open_private()

[Andrew Morton]

On Mon, 15 Sep 2014 08:21:45 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

> On 12/09/14 22:50, Andrew Morton wrote:
> > On Fri, 12 Sep 2014 15:09:36 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
> >
> >>    fs: proc: use __seq_open_private()
> >>    fs: proc: use __seq_open_private()
> >
> > See the problem?  We have two different patches, both named the same.
> 
> Always another gotcha! :-)
> 
> Seriously, does it say anywhere that patch names have to be unique? It
> makes perfect sense when it's pointed out but it never occurred to me.

Not explicitly as far as I know.  Documentation/SubmittingPatches
implies it.  Search for "unique".

Re: [PATCH 1/2] fs: proc: use __seq_open_private()

[Rob Jones]

On 12/09/14 22:54, Andrew Morton wrote:
> On Fri, 12 Sep 2014 15:09:37 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>> Reduce boilerplate code by using __seq_open_private() instead of seq_open().
>
> http://ozlabs.org/~akpm/mmots/broken-out/fs-proc-task_nommuc-change-maps_open-to-use-__seq_open_private.patch
> already did this.
>

Great. Too recent to be in my source tree though, I'm afraid.

Even so, it's nice to see that his code is identical to mine (except
for the assign-on-declaration, I prefer the whitespace, but hey).

I wonder if this was triggered by my documentation patch for the
routine. If so, that makes me happy :-)

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575

Re: [PATCH 1/2] fs: proc: use __seq_open_private()

[Rob Jones]

On 12/09/14 22:54, Andrew Morton wrote:
> On Fri, 12 Sep 2014 15:09:37 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>> Reduce boilerplate code by using __seq_open_private() instead of seq_open().
>
> http://ozlabs.org/~akpm/mmots/broken-out/fs-proc-task_nommuc-change-maps_open-to-use-__seq_open_private.patch
> already did this.

This duplicated only half of my submission, patch 2/2 did a similar
operation on a different file (fs/proc/task_mmu.c).

https://lkml.org/lkml/2014/9/12/347

Is that one actually going in (with Andrew's rename to include the file
name) or should I resubmit it on its own?

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones@codethink.co.uk
tel:+44 161 236 5575