[PATCH] seq_file: Allow private data to be supplied on seq_open

[PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

Create a function seq_open_priv() that is identical to seq_open() except
that it accepts a void * parameter that it stores in the private field
of the struct seq_file.

Document seq_open_priv().

Some consumers of the seq_file interface need to pass data to their
iterators that is best obtained at the time the seq_file is opened.

At the moment these consumers have to obtain the struct seq_file pointer
(stored by seq_open() in file->private_data) and then store a pointer to
their own data in the private field of the struct seq_file so that it
can be accessed by the iterator functions.

Although this is not a long piece of code it is unneccessary boilerplate.

seq_open() remains in place and its behaviour remains unchanged so no
existing code should be broken by this patch.

Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 Documentation/filesystems/seq_file.txt |    9 +++++++++
 fs/seq_file.c                          |   30 ++++++++++++++++++++++++++++--
 include/linux/seq_file.h               |    1 +
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
index a1e2e0d..128ffee 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -226,6 +226,15 @@ 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.
 
+There is also a function seq_open_priv() which behaves identically to
+seq_open() except that it takes an additional void * parameter that it
+stores in the private field of the seq_file structure, thereby making it
+available to the start function and thus all subsequent iterator functions
+Note that a corresponding wrapper function for seq_release() may need to
+be created to free any resources allocated by an open function that uses
+this capability (although, for simple cases, seq_release_private() may
+suffice).
+
 The other operations of interest - read(), llseek(), and release() - are
 all implemented by the seq_file code itself. So a virtual file's
 file_operations structure will look like:
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb..9a0db94 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -31,9 +31,10 @@ static void seq_set_overflow(struct seq_file *m)
 }
 
 /**
- *	seq_open -	initialize sequential file
+ *	seq_open_priv -	initialize sequential file with private data
  *	@file: file we initialize
  *	@op: method table describing the sequence
+ *	@d: private data to be made available to the iterator functions
  *
  *	seq_open() sets @file, associating it with a sequence described
  *	by @op.  @op->start() sets the iterator up and returns the first
@@ -43,8 +44,12 @@ static void seq_set_overflow(struct seq_file *m)
  *	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".
+ *
+ *	Supplying @d allows data that is only available at the time the file
+ *	is opened to be supplied to @op->start() (and thereby to @op->next()
+ *	as well).
  */
-int seq_open(struct file *file, const struct seq_operations *op)
+int seq_open_priv(struct file *file, const struct seq_operations *op, void *d)
 {
 	struct seq_file *p = file->private_data;
 
@@ -57,6 +62,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
 	memset(p, 0, sizeof(*p));
 	mutex_init(&p->lock);
 	p->op = op;
+	p->private = d;
 #ifdef CONFIG_USER_NS
 	p->user_ns = file->f_cred->user_ns;
 #endif
@@ -80,6 +86,26 @@ int seq_open(struct file *file, const struct seq_operations *op)
 	file->f_mode &= ~FMODE_PWRITE;
 	return 0;
 }
+EXPORT_SYMBOL(seq_open_priv);
+
+/**
+ *	seq_open -	initialize sequential file
+ *	@file: file we initialize
+ *	@op: method table describing the sequence
+ *
+ *	seq_open() 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
+ *	into the buffer.  In case of error ->start() and ->next() return
+ *	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".
+ */
+int seq_open(struct file *file, const struct seq_operations *op)
+{
+	return seq_open_priv(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 52e0097..fce87af 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -95,6 +95,7 @@ static inline void seq_setwidth(struct seq_file *m, size_t size)
 void seq_pad(struct seq_file *m, char c);
 
 char *mangle_path(char *s, const char *p, const char *esc);
+int seq_open_priv(struct file *, const struct seq_operations *, void *);
 int seq_open(struct file *, const struct seq_operations *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
 loff_t seq_lseek(struct file *, loff_t, int);
-- 
1.7.10.4

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

Does anyone have any feedback on this?

I would have thought it was quite uncontroversial.

On 29/07/14 18:39, Rob Jones wrote:
> Create a function seq_open_priv() that is identical to seq_open() except
> that it accepts a void * parameter that it stores in the private field
> of the struct seq_file.
>
> Document seq_open_priv().
>
> Some consumers of the seq_file interface need to pass data to their
> iterators that is best obtained at the time the seq_file is opened.
>
> At the moment these consumers have to obtain the struct seq_file pointer
> (stored by seq_open() in file->private_data) and then store a pointer to
> their own data in the private field of the struct seq_file so that it
> can be accessed by the iterator functions.
>
> Although this is not a long piece of code it is unneccessary boilerplate.
>
> seq_open() remains in place and its behaviour remains unchanged so no
> existing code should be broken by this patch.
>
> Signed-off-by: Ian Molton <ian.molton@codethink.co.uk>
> Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
> ---
>   Documentation/filesystems/seq_file.txt |    9 +++++++++
>   fs/seq_file.c                          |   30 ++++++++++++++++++++++++++++--
>   include/linux/seq_file.h               |    1 +
>   3 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index a1e2e0d..128ffee 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -226,6 +226,15 @@ 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.
>
> +There is also a function seq_open_priv() which behaves identically to
> +seq_open() except that it takes an additional void * parameter that it
> +stores in the private field of the seq_file structure, thereby making it
> +available to the start function and thus all subsequent iterator functions
> +Note that a corresponding wrapper function for seq_release() may need to
> +be created to free any resources allocated by an open function that uses
> +this capability (although, for simple cases, seq_release_private() may
> +suffice).
> +
>   The other operations of interest - read(), llseek(), and release() - are
>   all implemented by the seq_file code itself. So a virtual file's
>   file_operations structure will look like:
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1d641bb..9a0db94 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -31,9 +31,10 @@ static void seq_set_overflow(struct seq_file *m)
>   }
>
>   /**
> - *	seq_open -	initialize sequential file
> + *	seq_open_priv -	initialize sequential file with private data
>    *	@file: file we initialize
>    *	@op: method table describing the sequence
> + *	@d: private data to be made available to the iterator functions
>    *
>    *	seq_open() sets @file, associating it with a sequence described
>    *	by @op.  @op->start() sets the iterator up and returns the first
> @@ -43,8 +44,12 @@ static void seq_set_overflow(struct seq_file *m)
>    *	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".
> + *
> + *	Supplying @d allows data that is only available at the time the file
> + *	is opened to be supplied to @op->start() (and thereby to @op->next()
> + *	as well).
>    */
> -int seq_open(struct file *file, const struct seq_operations *op)
> +int seq_open_priv(struct file *file, const struct seq_operations *op, void *d)
>   {
>   	struct seq_file *p = file->private_data;
>
> @@ -57,6 +62,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
>   	memset(p, 0, sizeof(*p));
>   	mutex_init(&p->lock);
>   	p->op = op;
> +	p->private = d;
>   #ifdef CONFIG_USER_NS
>   	p->user_ns = file->f_cred->user_ns;
>   #endif
> @@ -80,6 +86,26 @@ int seq_open(struct file *file, const struct seq_operations *op)
>   	file->f_mode &= ~FMODE_PWRITE;
>   	return 0;
>   }
> +EXPORT_SYMBOL(seq_open_priv);
> +
> +/**
> + *	seq_open -	initialize sequential file
> + *	@file: file we initialize
> + *	@op: method table describing the sequence
> + *
> + *	seq_open() 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
> + *	into the buffer.  In case of error ->start() and ->next() return
> + *	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".
> + */
> +int seq_open(struct file *file, const struct seq_operations *op)
> +{
> +	return seq_open_priv(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 52e0097..fce87af 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -95,6 +95,7 @@ static inline void seq_setwidth(struct seq_file *m, size_t size)
>   void seq_pad(struct seq_file *m, char c);
>
>   char *mangle_path(char *s, const char *p, const char *esc);
> +int seq_open_priv(struct file *, const struct seq_operations *, void *);
>   int seq_open(struct file *, const struct seq_operations *);
>   ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
>   loff_t seq_lseek(struct file *, loff_t, int);
>

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

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Al Viro]

On Tue, Jul 29, 2014 at 06:39:53PM +0100, Rob Jones wrote:

> At the moment these consumers have to obtain the struct seq_file pointer
> (stored by seq_open() in file->private_data) and then store a pointer to
> their own data in the private field of the struct seq_file so that it
> can be accessed by the iterator functions.
> 
> Although this is not a long piece of code it is unneccessary boilerplate.

How many of those do we actually have?

> seq_open() remains in place and its behaviour remains unchanged so no
> existing code should be broken by this patch.

I have no objections against such helper, but I's rather have it
implemented via seq_open() (and as a static inline, not an export),
not the other way round.  Oh, and conversion of at least some users would
be nice to have as well...

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

On 06/08/14 17:02, Al Viro wrote:
> On Tue, Jul 29, 2014 at 06:39:53PM +0100, Rob Jones wrote:
>
>> At the moment these consumers have to obtain the struct seq_file pointer
>> (stored by seq_open() in file->private_data) and then store a pointer to
>> their own data in the private field of the struct seq_file so that it
>> can be accessed by the iterator functions.
>>
>> Although this is not a long piece of code it is unneccessary boilerplate.
>
> How many of those do we actually have?

A quick grep (I didn't examine them all) showed what looked like at
least 80 instances of the work around.

>
>> seq_open() remains in place and its behaviour remains unchanged so no
>> existing code should be broken by this patch.
>
> I have no objections against such helper, but I's rather have it
> implemented via seq_open() (and as a static inline, not an export),
> not the other way round.  Oh, and conversion of at least some users would
> be nice to have as well...
>
>

I did wonder about doing it as an inline but would argue that, for
efficiency, an external is better. There are some hundreds of calls to
seq_open(), each of which would need the compiler to insert the NULL
parameter, that seems like quite a few bytes of code added to the kernel
for the sake of saving one symbol export. However, if I'm wrong, I'll
happily change it to an inline, you guys have much more kernel
experience than me.

I'm not quite sure I understand your meaning when you say "via seq_open"
though, that function call format needs to stay the same or lots of
code will break, so I can't just add the third parameter on the end.
(C++ does have *some* advantages!) Can you clarify, please?

I was planning on changing a bunch of the instances after the patch went 
in. I can easily add a sample tidy up or two to the end, there's no
shortage of candidates.

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

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Eric W. Biederman]

Rob Jones <rob.jones@codethink.co.uk> writes:

> On 06/08/14 17:02, Al Viro wrote:
>> On Tue, Jul 29, 2014 at 06:39:53PM +0100, Rob Jones wrote:
>>
>>> At the moment these consumers have to obtain the struct seq_file pointer
>>> (stored by seq_open() in file->private_data) and then store a pointer to
>>> their own data in the private field of the struct seq_file so that it
>>> can be accessed by the iterator functions.
>>>
>>> Although this is not a long piece of code it is unneccessary boilerplate.
>>
>> How many of those do we actually have?
>
> A quick grep (I didn't examine them all) showed what looked like at
> least 80 instances of the work around.

I took a quick look as well and what I saw was that if we were to
implement the helpers: seq_open_PDE_DATA, and seq_open_i_private.  That
would cover essentially all of seq_open that set seq_file->private.  So
my gut feel is that a seq_open_priv is the wrong helper.

In the vast majority of the cases either seq_open is good enough,
we want seq_open_private, or seq_file->private is set to PDE_DATA
or i_private.

I think there may be 5 cases in the kernel that do something different,
and those cases probably need a code audit.

>>> seq_open() remains in place and its behaviour remains unchanged so no
>>> existing code should be broken by this patch.
>>
>> I have no objections against such helper, but I's rather have it
>> implemented via seq_open() (and as a static inline, not an export),
>> not the other way round.  Oh, and conversion of at least some users would
>> be nice to have as well...

I have no significant objection but having both seq_open_private
and seq_open_priv seems confusing name wise.

Eric

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Al Viro]

On Wed, Aug 06, 2014 at 05:16:49PM +0100, Rob Jones wrote:

> I'm not quite sure I understand your meaning when you say "via seq_open"
> though, that function call format needs to stay the same or lots of
> code will break, so I can't just add the third parameter on the end.
> (C++ does have *some* advantages!) Can you clarify, please?

seq_open_private() can be implemented as call of seq_open() + assignment...

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Wed, Aug 06, 2014 at 05:16:49PM +0100, Rob Jones wrote:
>
>> I'm not quite sure I understand your meaning when you say "via seq_open"
>> though, that function call format needs to stay the same or lots of
>> code will break, so I can't just add the third parameter on the end.
>> (C++ does have *some* advantages!) Can you clarify, please?
>
> seq_open_private() can be implemented as call of seq_open() +
> assignment...

This is why I object to the name seq_open_priv() for the new code.
seq_open_private() is already implemented and base on seq_open().

And the names are close enough together it is confusing :(

Eric

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

On 06/08/14 20:14, Eric W. Biederman wrote:
> Rob Jones <rob.jones@codethink.co.uk> writes:
>
>> On 06/08/14 17:02, Al Viro wrote:
>>> On Tue, Jul 29, 2014 at 06:39:53PM +0100, Rob Jones wrote:
>>>
>>>> At the moment these consumers have to obtain the struct seq_file pointer
>>>> (stored by seq_open() in file->private_data) and then store a pointer to
>>>> their own data in the private field of the struct seq_file so that it
>>>> can be accessed by the iterator functions.
>>>>
>>>> Although this is not a long piece of code it is unneccessary boilerplate.
>>>
>>> How many of those do we actually have?
>>
>> A quick grep (I didn't examine them all) showed what looked like at
>> least 80 instances of the work around.
>
> I took a quick look as well and what I saw was that if we were to
> implement the helpers: seq_open_PDE_DATA, and seq_open_i_private.  That
> would cover essentially all of seq_open that set seq_file->private.  So
> my gut feel is that a seq_open_priv is the wrong helper.
>
> In the vast majority of the cases either seq_open is good enough,
> we want seq_open_private, or seq_file->private is set to PDE_DATA
> or i_private.

I completely missed seq_open_private().

Maybe because it is completely undocumented.

Having partial documentation can be worse than having no documentation
at all - if there had been no documentation I would have been obliged to
search through the code. Unfortunately, this is both tiresome and time
consuming and, given that there is a documentation file, should not be
necessary.

On a related subject, Having looked at a few uses of seq_file, I must
say that some users seem to make assumptions about the internal
workings of the module. Dangerous behaviour as only some behaviours are
documented.

e.g. The behaviour that "struct seq_file" pointer is stored in
file->private_data is documented and can therefore be relied upon but
the fact that the output buffer and its size are only defined at the
first output (and can therefore be pre-defined and pre-allocated by
user code) is not documented and could therefore change without warning.

This second behaviour is assumed in, for example, module fs/gfs2/glock.c
which could, therefore, stop working properly without warning if the
internal behaviour was changed.

>
> I think there may be 5 cases in the kernel that do something different,
> and those cases probably need a code audit.
>
>>>> seq_open() remains in place and its behaviour remains unchanged so no
>>>> existing code should be broken by this patch.
>>>
>>> I have no objections against such helper, but I's rather have it
>>> implemented via seq_open() (and as a static inline, not an export),
>>> not the other way round.  Oh, and conversion of at least some users would
>>> be nice to have as well...
>
> I have no significant objection but having both seq_open_private
> and seq_open_priv seems confusing name wise.

Rather than pursuing my original patch, I think I will document
seq_open_private() as it achieves the same objective in a slightly
different way. Had I found it initially I almost certainly wouldn't
have created the patch.

>
> Eric

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

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Steven Whitehouse]

Hi,

On 07/08/14 13:58, Rob Jones wrote:
[snip]
>
> On a related subject, Having looked at a few uses of seq_file, I must
> say that some users seem to make assumptions about the internal
> workings of the module. Dangerous behaviour as only some behaviours are
> documented.
>
> e.g. The behaviour that "struct seq_file" pointer is stored in
> file->private_data is documented and can therefore be relied upon but
> the fact that the output buffer and its size are only defined at the
> first output (and can therefore be pre-defined and pre-allocated by
> user code) is not documented and could therefore change without warning.
>
> This second behaviour is assumed in, for example, module fs/gfs2/glock.c
> which could, therefore, stop working properly without warning if the
> internal behaviour was changed.
>
While it is undocumented, it is I understand, how this feature was 
intended to be used, so I think that it is safe to do this in the GFS2 
case. Here is a ref to the thread which explains how it landed up like that:
https://www.redhat.com/archives/cluster-devel/2012-June/msg00000.html

Steve.

Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

Hi Steve,

On 07/08/14 14:32, Steven Whitehouse wrote:
> Hi,
>
> On 07/08/14 13:58, Rob Jones wrote:
> [snip]
>>
>> On a related subject, Having looked at a few uses of seq_file, I must
>> say that some users seem to make assumptions about the internal
>> workings of the module. Dangerous behaviour as only some behaviours are
>> documented.
>>
>> e.g. The behaviour that "struct seq_file" pointer is stored in
>> file->private_data is documented and can therefore be relied upon but
>> the fact that the output buffer and its size are only defined at the
>> first output (and can therefore be pre-defined and pre-allocated by
>> user code) is not documented and could therefore change without warning.
>>
>> This second behaviour is assumed in, for example, module fs/gfs2/glock.c
>> which could, therefore, stop working properly without warning if the
>> internal behaviour was changed.
>>
> While it is undocumented, it is I understand, how this feature was
> intended to be used, so I think that it is safe to do this in the GFS2
> case. Here is a ref to the thread which explains how it landed up like
> that:
> https://www.redhat.com/archives/cluster-devel/2012-June/msg00000.html

No criticism was intended of that particular piece of code, It has been
there for a couple of years and is, presumably, still working :-)

It was just a general point about things needing to be written down. A
behaviour such as you were relying on can be a very positive thing but
it would be of much greater use if it was written down in the file docs.

I completely missed seq_file_private() because I was looking at the
docs more than the code. If it had been written down in the docs it
would have saved me quite a bit of time, similarly, if the buffer
allocation behaviour was documented, changes to seq_file.c would not be
made that could break your code.

God knows, I'm not a fan of unnecessary documentation but where it's
useful I'm all for it.

>
> Steve.

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

Re: [Linux-kernel] [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

On 07/08/14 15:09, Rob Jones wrote:
> Hi Steve,
>
> On 07/08/14 14:32, Steven Whitehouse wrote:
>> Hi,
>>
>> On 07/08/14 13:58, Rob Jones wrote:
>> [snip]
>>>
>>> On a related subject, Having looked at a few uses of seq_file, I must
>>> say that some users seem to make assumptions about the internal
>>> workings of the module. Dangerous behaviour as only some behaviours are
>>> documented.
>>>
>>> e.g. The behaviour that "struct seq_file" pointer is stored in
>>> file->private_data is documented and can therefore be relied upon but
>>> the fact that the output buffer and its size are only defined at the
>>> first output (and can therefore be pre-defined and pre-allocated by
>>> user code) is not documented and could therefore change without warning.
>>>
>>> This second behaviour is assumed in, for example, module fs/gfs2/glock.c
>>> which could, therefore, stop working properly without warning if the
>>> internal behaviour was changed.
>>>
>> While it is undocumented, it is I understand, how this feature was
>> intended to be used, so I think that it is safe to do this in the GFS2
>> case. Here is a ref to the thread which explains how it landed up like
>> that:
>> https://www.redhat.com/archives/cluster-devel/2012-June/msg00000.html
>
> No criticism was intended of that particular piece of code, It has been
> there for a couple of years and is, presumably, still working :-)
>
> It was just a general point about things needing to be written down. A
> behaviour such as you were relying on can be a very positive thing but
> it would be of much greater use if it was written down in the file docs.
>
> I completely missed seq_file_private() because I was looking at the

Sorry, that should be seq_open_private()

Why does one never see the mistake until *after* hitting send?

> docs more than the code. If it had been written down in the docs it
> would have saved me quite a bit of time, similarly, if the buffer
> allocation behaviour was documented, changes to seq_file.c would not be
> made that could break your code.
>
> God knows, I'm not a fan of unnecessary documentation but where it's
> useful I'm all for it.
>
>>
>> Steve.
>

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

Re: [Linux-kernel] [PATCH] seq_file: Allow private data to be supplied on seq_open

[Steven Whitehouse]

Hi,

On 07/08/14 15:16, Rob Jones wrote:
>
>
> On 07/08/14 15:09, Rob Jones wrote:
>> Hi Steve,
>>
>> On 07/08/14 14:32, Steven Whitehouse wrote:
>>> Hi,
>>>
>>> On 07/08/14 13:58, Rob Jones wrote:
>>> [snip]
>>>>
>>>> On a related subject, Having looked at a few uses of seq_file, I must
>>>> say that some users seem to make assumptions about the internal
>>>> workings of the module. Dangerous behaviour as only some behaviours 
>>>> are
>>>> documented.
>>>>
>>>> e.g. The behaviour that "struct seq_file" pointer is stored in
>>>> file->private_data is documented and can therefore be relied upon but
>>>> the fact that the output buffer and its size are only defined at the
>>>> first output (and can therefore be pre-defined and pre-allocated by
>>>> user code) is not documented and could therefore change without 
>>>> warning.
>>>>
>>>> This second behaviour is assumed in, for example, module 
>>>> fs/gfs2/glock.c
>>>> which could, therefore, stop working properly without warning if the
>>>> internal behaviour was changed.
>>>>
>>> While it is undocumented, it is I understand, how this feature was
>>> intended to be used, so I think that it is safe to do this in the GFS2
>>> case. Here is a ref to the thread which explains how it landed up like
>>> that:
>>> https://www.redhat.com/archives/cluster-devel/2012-June/msg00000.html
>>
>> No criticism was intended of that particular piece of code, It has been
>> there for a couple of years and is, presumably, still working :-)
>>
>> It was just a general point about things needing to be written down. A
>> behaviour such as you were relying on can be a very positive thing but
>> it would be of much greater use if it was written down in the file docs.
>>
>> I completely missed seq_file_private() because I was looking at the
>
> Sorry, that should be seq_open_private()
>
> Why does one never see the mistake until *after* hitting send?
>
Always the way, unfortunately!

>> docs more than the code. If it had been written down in the docs it
>> would have saved me quite a bit of time, similarly, if the buffer
>> allocation behaviour was documented, changes to seq_file.c would not be
>> made that could break your code.
>>
>> God knows, I'm not a fan of unnecessary documentation but where it's
>> useful I'm all for it.
>>
Yes, very much agreed, and no doubt it would be useful in this case. I 
hoped that the earlier thread might be a useful starting point, since it 
explained some of the whys and wherefores,

Steve.

Re: [Linux-kernel] [PATCH] seq_file: Allow private data to be supplied on seq_open

[Rob Jones]

On 07/08/14 15:22, Steven Whitehouse wrote:
> Hi,
>
> On 07/08/14 15:16, Rob Jones wrote:
>>
>>
>> On 07/08/14 15:09, Rob Jones wrote:
>>> Hi Steve,
>>>
>>> On 07/08/14 14:32, Steven Whitehouse wrote:
>>>> Hi,
>>>>
>>>> On 07/08/14 13:58, Rob Jones wrote:
>>>> [snip]
>>>>>
>>>>> On a related subject, Having looked at a few uses of seq_file, I must
>>>>> say that some users seem to make assumptions about the internal
>>>>> workings of the module. Dangerous behaviour as only some behaviours
>>>>> are
>>>>> documented.
>>>>>
>>>>> e.g. The behaviour that "struct seq_file" pointer is stored in
>>>>> file->private_data is documented and can therefore be relied upon but
>>>>> the fact that the output buffer and its size are only defined at the
>>>>> first output (and can therefore be pre-defined and pre-allocated by
>>>>> user code) is not documented and could therefore change without
>>>>> warning.
>>>>>
>>>>> This second behaviour is assumed in, for example, module
>>>>> fs/gfs2/glock.c
>>>>> which could, therefore, stop working properly without warning if the
>>>>> internal behaviour was changed.
>>>>>
>>>> While it is undocumented, it is I understand, how this feature was
>>>> intended to be used, so I think that it is safe to do this in the GFS2
>>>> case. Here is a ref to the thread which explains how it landed up like
>>>> that:
>>>> https://www.redhat.com/archives/cluster-devel/2012-June/msg00000.html
>>>
>>> No criticism was intended of that particular piece of code, It has been
>>> there for a couple of years and is, presumably, still working :-)
>>>
>>> It was just a general point about things needing to be written down. A
>>> behaviour such as you were relying on can be a very positive thing but
>>> it would be of much greater use if it was written down in the file docs.
>>>
>>> I completely missed seq_file_private() because I was looking at the
>>
>> Sorry, that should be seq_open_private()
>>
>> Why does one never see the mistake until *after* hitting send?
>>
> Always the way, unfortunately!
>
>>> docs more than the code. If it had been written down in the docs it
>>> would have saved me quite a bit of time, similarly, if the buffer
>>> allocation behaviour was documented, changes to seq_file.c would not be
>>> made that could break your code.
>>>
>>> God knows, I'm not a fan of unnecessary documentation but where it's
>>> useful I'm all for it.
>>>
> Yes, very much agreed, and no doubt it would be useful in this case. I
> hoped that the earlier thread might be a useful starting point, since it
> explained some of the whys and wherefores,

Well, I'm making a start by documenting seq_open_private(). Small
steps :-)

>
> Steve.
>
>
>

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