linux-kernel - Re: [PATCH] seq_file: Allow private data to be supplied on seq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53E254F1.30605@codethink.co.uk>
Date:	Wed, 06 Aug 2014 17:16:49 +0100
From:	Rob Jones <rob.jones@...ethink.co.uk>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kernel@...ts.codethink.co.uk,
	ebiederm@...ssion.com, ian.molton@...ethink.co.uk
Subject: Re: [PATCH] seq_file: Allow private data to be supplied on seq_open

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@...ethink.co.uk
tel:+44 161 236 5575
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/