From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH] seq_file: Allow private data to be supplied on seq_open Date: Wed, 06 Aug 2014 12:14:51 -0700 Message-ID: <87sil9sa50.fsf@x220.int.ebiederm.org> References: <1406655593-12626-1-git-send-email-rob.jones@codethink.co.uk> <20140806160259.GR18016@ZenIV.linux.org.uk> <53E254F1.30605@codethink.co.uk> Mime-Version: 1.0 Content-Type: text/plain Cc: Al Viro , linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-kernel@lists.codethink.co.uk, ian.molton@codethink.co.uk To: Rob Jones Return-path: Received: from out01.mta.xmission.com ([166.70.13.231]:40367 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932234AbaHFTSq (ORCPT ); Wed, 6 Aug 2014 15:18:46 -0400 In-Reply-To: <53E254F1.30605@codethink.co.uk> (Rob Jones's message of "Wed, 06 Aug 2014 17:16:49 +0100") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Rob Jones 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