[PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Rob Jones]

Series resubmitted due to a typo in an email address.

This patch series implements and documents a new interface function for
seq_file.

The existing set of open functions: seq_open(), seq_open_private() and
__seq_open_private() satisfy the majority of use cases however there is
one more use case that is also very common that this new function
addresses.

This case is where the iterator needs information that is available only at
the time the seq_file is opened but does not need any space allocated, e.g.
access to the inode structure. This type of open occurs, by my best estimate,
in well over 40 places. 

Using the new function saves at least two lines of boilerplate code per
instance as well as making the code easier to follow. The additional code
in seq_file.c to implement the function is minimal as the first place that
code can be removed is within seq_file.c itself.

Once this patch is accepted, the instances of boilerplate code can be
addressed.

The documentation of seq_open() and its variants has been re-worked to try
to guide users towards the most appropriate variant for their application.

Rob Jones (2):
  fs/seq_file: Create new function seq_open_init()
  Documentation/filesystem/seq_file: document seq_open_init()

 Documentation/filesystems/seq_file.txt |   58 +++++++++++++++++++++-----------
 fs/seq_file.c                          |   34 ++++++++++++-------
 include/linux/seq_file.h               |    1 +
 3 files changed, 62 insertions(+), 31 deletions(-)

-- 
1.7.10.4

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

[Rob Jones]

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.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 fs/seq_file.c            |   34 ++++++++++++++++++++++------------
 include/linux/seq_file.h |    1 +
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index dc2dfec..4be3aa8 100644
--- 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);
+
 void *__seq_open_private(struct file *f, const struct seq_operations *ops,
 		size_t psize)
 {
 	int rc;
 	void *private;
-	struct seq_file *seq;
 
 	private = kzalloc(psize, GFP_KERNEL);
-	if (private == NULL)
-		goto out;
+	if (!private)
+		return NULL;
 
-	rc = seq_open(f, ops);
-	if (rc < 0)
-		goto out_free;
-
-	seq = f->private_data;
-	seq->private = private;
-	return private;
+	rc = seq_open_init(f, ops, private);
+	if (!rc)
+		return private;
 
-out_free:
 	kfree(private);
-out:
+
 	return NULL;
 }
 EXPORT_SYMBOL(__seq_open_private);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 9382339..6b0d953 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -142,6 +142,7 @@ int single_open_size(struct file *, int (*)(struct seq_file *, void *), void *,
 int single_release(struct inode *, struct file *);
 void *__seq_open_private(struct file *, const struct seq_operations *, size_t);
 int seq_open_private(struct file *, const struct seq_operations *, size_t);
+int seq_open_init(struct file *, const struct seq_operations *, void *);
 int seq_release_private(struct inode *, struct file *);
 int seq_put_decimal_ull(struct seq_file *m, char delimiter,
 			unsigned long long num);
-- 
1.7.10.4

[PATCH RESUBMIT 2/2] Documentation/filesystem/seq_file: document seq_open_init()

[Rob Jones]

Add documentation for new function and restructure existing text
in the same area.

Signed-off-by: Rob Jones <rob.jones@codethink.co.uk>
---
 Documentation/filesystems/seq_file.txt |   58 +++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
index 420fc0d..10a3be6 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -221,15 +221,37 @@ 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.
 
-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
-private field of the seq_file structure, returning 0 on success. The
-block size is specified in a third parameter to the function, e.g.:
+Many applications can use the same iterator 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.
+
+To facilitate this, a number of wrapper functions to seq_open() are
+provided:
+
++----------------------+---------------------------------------------------------+
+|      Function        | Use case                                                |
++----------------------+---------------------------------------------------------+
+| seq_open()           | Iterator needs no pre-initialised data                  |
+| seq_open_init()      | Iterator needs a pointer to data but no kmalloc needed  |
+| seq_open_private()   | Iterator needs data but kzalloc() suffices              |
+| __seq_open_private() | Iterator needs data with initialisation after kzalloc() |
++----------------------+---------------------------------------------------------+
+
+seq_open_init() is similiar to seq_open() except that it accepts a third
+parameter of type (void *) which it stores in the private field of the
+seq_file structure, e.g.:
+
+	static int ct_open(struct inode *inode, struct file *file)
+	{
+		return seq_open_init(file, &ct_seq_ops, &mystruct);
+	}
+
+seq_open_private() is similar to seq_open_init() except that the third
+parameter is a size. The function kmallocs a zero filled block of memory
+of the supplied size and stores a pointer to this block in the private
+field of the seq_file structure, returning 0 on success, e.g.:
 
 	static int ct_open(struct inode *inode, struct file *file)
 	{
@@ -237,15 +259,14 @@ block size is specified in a third parameter to the function, e.g.:
 					sizeof(struct mystruct));
 	}
 
-There is also a variant function, __seq_open_private(), which is functionally
-identical except that, if successful, it returns the pointer to the allocated
-memory block, allowing further initialisation e.g.:
+__seq_open_private()is a variant of seq_open_private(), functionally
+identical except that, if successful, it returns the pointer to the
+allocated memory block, allowing further initialisation, e.g.:
 
 	static int ct_open(struct inode *inode, struct file *file)
 	{
-		struct mystruct *p =
-			__seq_open_private(file, &ct_seq_ops, sizeof(*p));
-
+		struct mystruct *p;
+		p = __seq_open_private(file, &ct_seq_ops, sizeof(*p));
 		if (!p)
 			return -ENOMEM;
 
@@ -256,9 +277,6 @@ memory block, allowing further initialisation e.g.:
 		return 0;
 	}
 
-A corresponding close function, seq_release_private() is available which
-frees the memory allocated in the corresponding open.
-
 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:
@@ -271,8 +289,10 @@ file_operations structure will look like:
 	        .release = seq_release
 	};
 
-There is also a seq_release_private() which passes the contents of the
-seq_file private field to kfree() before releasing the structure.
+There is also wrapper function, seq_release_private(), which can be used
+instead of seq_release(). It is identical except that it passes the contents
+of the seq_file private field to kfree() before releasing the seq_file
+structure itself.
 
 The final step is the creation of the /proc file itself. In the example
 code, that is done in the initialization code in the usual way:
-- 
1.7.10.4

Re: [PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Kees Cook]

On Wed, Sep 24, 2014 at 4:15 AM, Rob Jones <rob.jones@codethink.co.uk> wrote:
> Series resubmitted due to a typo in an email address.
>
> This patch series implements and documents a new interface function for
> seq_file.
>
> The existing set of open functions: seq_open(), seq_open_private() and
> __seq_open_private() satisfy the majority of use cases however there is
> one more use case that is also very common that this new function
> addresses.
>
> This case is where the iterator needs information that is available only at
> the time the seq_file is opened but does not need any space allocated, e.g.
> access to the inode structure. This type of open occurs, by my best estimate,
> in well over 40 places.
>
> Using the new function saves at least two lines of boilerplate code per
> instance as well as making the code easier to follow. The additional code
> in seq_file.c to implement the function is minimal as the first place that
> code can be removed is within seq_file.c itself.
>
> Once this patch is accepted, the instances of boilerplate code can be
> addressed.

Would it be possible to write a coccinelle patch for the replacements?

This seems like a good idea. Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> The documentation of seq_open() and its variants has been re-worked to try
> to guide users towards the most appropriate variant for their application.
>
> Rob Jones (2):
>   fs/seq_file: Create new function seq_open_init()
>   Documentation/filesystem/seq_file: document seq_open_init()
>
>  Documentation/filesystems/seq_file.txt |   58 +++++++++++++++++++++-----------
>  fs/seq_file.c                          |   34 ++++++++++++-------
>  include/linux/seq_file.h               |    1 +
>  3 files changed, 62 insertions(+), 31 deletions(-)
>
> --
> 1.7.10.4
>



-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Wed, 24 Sep 2014 12:15:55 +0100 Rob Jones <rob.jones@codethink.co.uk> 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.

> --- 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.


__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.

Re: [PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Rob Jones]

On 24/09/14 19:06, Kees Cook wrote:
> On Wed, Sep 24, 2014 at 4:15 AM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>> Series resubmitted due to a typo in an email address.
>>
>> This patch series implements and documents a new interface function for
>> seq_file.
>>
>> The existing set of open functions: seq_open(), seq_open_private() and
>> __seq_open_private() satisfy the majority of use cases however there is
>> one more use case that is also very common that this new function
>> addresses.
>>
>> This case is where the iterator needs information that is available only at
>> the time the seq_file is opened but does not need any space allocated, e.g.
>> access to the inode structure. This type of open occurs, by my best estimate,
>> in well over 40 places.
>>
>> Using the new function saves at least two lines of boilerplate code per
>> instance as well as making the code easier to follow. The additional code
>> in seq_file.c to implement the function is minimal as the first place that
>> code can be removed is within seq_file.c itself.
>>
>> Once this patch is accepted, the instances of boilerplate code can be
>> addressed.
>
> Would it be possible to write a coccinelle patch for the replacements?

I'm afraid I don't know what that means.

>
> This seems like a good idea. Thanks!
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
>>
>> The documentation of seq_open() and its variants has been re-worked to try
>> to guide users towards the most appropriate variant for their application.
>>
>> Rob Jones (2):
>>    fs/seq_file: Create new function seq_open_init()
>>    Documentation/filesystem/seq_file: document seq_open_init()
>>
>>   Documentation/filesystems/seq_file.txt |   58 +++++++++++++++++++++-----------
>>   fs/seq_file.c                          |   34 ++++++++++++-------
>>   include/linux/seq_file.h               |    1 +
>>   3 files changed, 62 insertions(+), 31 deletions(-)
>>
>> --
>> 1.7.10.4
>>
>
>
>

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

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

[Rob Jones]

On 24/09/14 22:39, Andrew Morton wrote:
> On Wed, 24 Sep 2014 12:15:55 +0100 Rob Jones <rob.jones@codethink.co.uk> 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@codethink.co.uk
tel:+44 161 236 5575

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

[Randy Dunlap]

On 09/25/14 02:10, Rob Jones wrote:
> 
> 
> On 24/09/14 22:39, Andrew Morton wrote:
>> On Wed, 24 Sep 2014 12:15:55 +0100 Rob Jones <rob.jones@codethink.co.uk> 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.

Whoever merges the fs/ changes can (should) also merge the Documentation changes.

-- 
~Randy

Re: [PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Kees Cook]

On Thu, Sep 25, 2014 at 1:57 AM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>
> On 24/09/14 19:06, Kees Cook wrote:
>>
>> On Wed, Sep 24, 2014 at 4:15 AM, Rob Jones <rob.jones@codethink.co.uk>
>> wrote:
>>>
>>> Series resubmitted due to a typo in an email address.
>>>
>>> This patch series implements and documents a new interface function for
>>> seq_file.
>>>
>>> The existing set of open functions: seq_open(), seq_open_private() and
>>> __seq_open_private() satisfy the majority of use cases however there is
>>> one more use case that is also very common that this new function
>>> addresses.
>>>
>>> This case is where the iterator needs information that is available only
>>> at
>>> the time the seq_file is opened but does not need any space allocated,
>>> e.g.
>>> access to the inode structure. This type of open occurs, by my best
>>> estimate,
>>> in well over 40 places.
>>>
>>> Using the new function saves at least two lines of boilerplate code per
>>> instance as well as making the code easier to follow. The additional code
>>> in seq_file.c to implement the function is minimal as the first place
>>> that
>>> code can be removed is within seq_file.c itself.
>>>
>>> Once this patch is accepted, the instances of boilerplate code can be
>>> addressed.
>>
>>
>> Would it be possible to write a coccinelle patch for the replacements?
>
>
> I'm afraid I don't know what that means.

It's a very flexible tool that should be able to find all the places
where this pattern is being used, and you can replace it with the new
function call:

http://lwn.net/Articles/315686/

-Kees


-- 
Kees Cook
Chrome OS Security

Re: [PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Rob Jones]

On 25/09/14 17:09, Kees Cook wrote:
> On Thu, Sep 25, 2014 at 1:57 AM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>>
>>
>> On 24/09/14 19:06, Kees Cook wrote:
>>>
>>> On Wed, Sep 24, 2014 at 4:15 AM, Rob Jones <rob.jones@codethink.co.uk>
>>> wrote:
>>>>
>>>> Series resubmitted due to a typo in an email address.
>>>>
>>>> This patch series implements and documents a new interface function for
>>>> seq_file.
>>>>
>>>> The existing set of open functions: seq_open(), seq_open_private() and
>>>> __seq_open_private() satisfy the majority of use cases however there is
>>>> one more use case that is also very common that this new function
>>>> addresses.
>>>>
>>>> This case is where the iterator needs information that is available only
>>>> at
>>>> the time the seq_file is opened but does not need any space allocated,
>>>> e.g.
>>>> access to the inode structure. This type of open occurs, by my best
>>>> estimate,
>>>> in well over 40 places.
>>>>
>>>> Using the new function saves at least two lines of boilerplate code per
>>>> instance as well as making the code easier to follow. The additional code
>>>> in seq_file.c to implement the function is minimal as the first place
>>>> that
>>>> code can be removed is within seq_file.c itself.
>>>>
>>>> Once this patch is accepted, the instances of boilerplate code can be
>>>> addressed.
>>>
>>>
>>> Would it be possible to write a coccinelle patch for the replacements?
>>
>>
>> I'm afraid I don't know what that means.
>
> It's a very flexible tool that should be able to find all the places
> where this pattern is being used, and you can replace it with the new
> function call:
>
> http://lwn.net/Articles/315686/

I suspect that the learning curve would exceed the utility but I'll have
a look at it. Unless there's a coccinelle expert available to do it, in
which case I could point them in the right direction.

My gut reaction would be that by the time I had analysed enough cases
to come up with a viable set of SmPL scripts I would have done most of
the work required, especially if I had to learn a new syntax and tool
to do it.

But first impressions aren't always right.

>
> -Kees
>
>

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

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

[Rob Jones]

On 25/09/14 15:49, Randy Dunlap wrote:
> On 09/25/14 02:10, Rob Jones wrote:
>>
>>
>> On 24/09/14 22:39, Andrew Morton wrote:
>>> On Wed, 24 Sep 2014 12:15:55 +0100 Rob Jones <rob.jones@codethink.co.uk> 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.
>
> Whoever merges the fs/ changes can (should) also merge the Documentation changes.

OK, if I resubmit (which seems quite likely), I'll merge them into a
single patch.

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

Re: [PATCH RESUBMIT 0/2] fs/seq_file: Add seq_open_init()

[Kees Cook]

On Thu, Sep 25, 2014 at 10:36 AM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>
> On 25/09/14 17:09, Kees Cook wrote:
>>
>> On Thu, Sep 25, 2014 at 1:57 AM, Rob Jones <rob.jones@codethink.co.uk>
>> wrote:
>>>
>>>
>>>
>>> On 24/09/14 19:06, Kees Cook wrote:
>>>>
>>>>
>>>> On Wed, Sep 24, 2014 at 4:15 AM, Rob Jones <rob.jones@codethink.co.uk>
>>>> wrote:
>>>>>
>>>>>
>>>>> Series resubmitted due to a typo in an email address.
>>>>>
>>>>> This patch series implements and documents a new interface function for
>>>>> seq_file.
>>>>>
>>>>> The existing set of open functions: seq_open(), seq_open_private() and
>>>>> __seq_open_private() satisfy the majority of use cases however there is
>>>>> one more use case that is also very common that this new function
>>>>> addresses.
>>>>>
>>>>> This case is where the iterator needs information that is available
>>>>> only
>>>>> at
>>>>> the time the seq_file is opened but does not need any space allocated,
>>>>> e.g.
>>>>> access to the inode structure. This type of open occurs, by my best
>>>>> estimate,
>>>>> in well over 40 places.
>>>>>
>>>>> Using the new function saves at least two lines of boilerplate code per
>>>>> instance as well as making the code easier to follow. The additional
>>>>> code
>>>>> in seq_file.c to implement the function is minimal as the first place
>>>>> that
>>>>> code can be removed is within seq_file.c itself.
>>>>>
>>>>> Once this patch is accepted, the instances of boilerplate code can be
>>>>> addressed.
>>>>
>>>>
>>>>
>>>> Would it be possible to write a coccinelle patch for the replacements?
>>>
>>>
>>>
>>> I'm afraid I don't know what that means.
>>
>>
>> It's a very flexible tool that should be able to find all the places
>> where this pattern is being used, and you can replace it with the new
>> function call:
>>
>> http://lwn.net/Articles/315686/
>
>
> I suspect that the learning curve would exceed the utility but I'll have
> a look at it. Unless there's a coccinelle expert available to do it, in
> which case I could point them in the right direction.
>
> My gut reaction would be that by the time I had analysed enough cases
> to come up with a viable set of SmPL scripts I would have done most of
> the work required, especially if I had to learn a new syntax and tool
> to do it.
>
> But first impressions aren't always right.

No worries; I was just curious if it would be easy. Sounds like
"probably not", which is fine. :)

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Thu, 25 Sep 2014 10:10:05 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

> > 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.

A single patch would be OK.

Documentation/ is nice, but I don't think people think to look there. 
Some kerneldoc within the .c would be a good addition.

> > __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.

I guess switch this patch to use "private" then a second one to fix
single_open().

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

[Rob Jones]

On 25/09/14 18:50, Andrew Morton wrote:
> On Thu, 25 Sep 2014 10:10:05 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>>> 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.
>
> A single patch would be OK.
>
> Documentation/ is nice, but I don't think people think to look there.
> Some kerneldoc within the .c would be a good addition.

Now is a good time, can you point me at an instance of good practice of
this?

>
>>> __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.
>
> I guess switch this patch to use "private" then a second one to fix
> single_open().
>
>
>

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

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

[Andrew Morton]

On Thu, 25 Sep 2014 18:54:40 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:

> On 25/09/14 18:50, Andrew Morton wrote:
> > On Thu, 25 Sep 2014 10:10:05 +0100 Rob Jones <rob.jones@codethink.co.uk> wrote:
> >
> >>> 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.
> >
> > A single patch would be OK.
> >
> > Documentation/ is nice, but I don't think people think to look there.
> > Some kerneldoc within the .c would be a good addition.
> 
> Now is a good time, can you point me at an instance of good practice of
> this?

Search fs/seq_file.c for "^/**"?