Re: [PATCH RESUBMIT 1/2] fs/seq_file: Create new function seq_open_init()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 24/09/14 22:39, Andrew Morton wrote:
On Wed, 24 Sep 2014 12:15:55 +0100 Rob Jones <rob.jones@xxxxxxxxxxxxxxx> wrote:

Add a new function to help reduce boilerplate code.

This is a wrapper function for seq_open() that will simplify the code in a
significant number of cases where seq_open() is currently called.

It's first use is in __seq_open_private(), thereby recovering most of
the code space used by the new function.

It would be nice to include one or more of the conversions in this patch
series so we can see what the effects look like.

There are certainly lots of candidates around. However, I thought that
the change to __seq_open_private() already gave a good illustration of
the level of savings to be made, in that it more or less made the new
function "self financing".


--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -639,28 +639,38 @@ int seq_release_private(struct inode *inode, struct file *file)
  }
  EXPORT_SYMBOL(seq_release_private);

+int seq_open_init(struct file *f, const struct seq_operations *ops, void *p)
+{
+	struct seq_file *s;
+	int rc;
+
+	rc = seq_open(f, ops);
+	if (rc)
+		return rc;
+
+	s = f->private_data;
+	s->private = p;
+
+	return 0;
+}
+EXPORT_SYMBOL(seq_open_init);

A global exported-to-modules interface should be documented, please.
Especially when it has a void* argument.  seq_file.c is patchy - some
of it is documented, some of it uses the read-programmers-mind
approach.

I have included documentation as the second patch. Would it have been
better to include them in a single patch? I didn't do that because
seq_file and Documentation have different maintainers. I'm still
learning the protocols here.



__seq_open_private() has
	void *private;

single_open() has
	void *data

And now seq_open_init() has
	void *p

but these all refer to the same thing.  Can we have a bit of
consistency in the naming please?  I suggest "private", to match
the seq_file field.

A valid point and I can easily make the change but fixing single_open()
would mean that the patch is addressing two issues, is that acceptable?
Another protocol question, sorry.

--
Rob Jones
Codethink Ltd
mailto:rob.jones@xxxxxxxxxxxxxxx
tel:+44 161 236 5575
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux