Messages in this thread | | | From | (Eric W. Biederman) | Date | Wed, 06 Aug 2014 12:14:51 -0700 | Subject | Re: [PATCH] seq_file: Allow private data to be supplied on seq_open |
| |
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
| |