[PATCH] fs.h: introduce functions to get/set file->private_data

[PATCH] fs.h: introduce functions to get/set file->private_data

[H Hartley Sweeten]

The symbol 'private_data' is commonly used and makes grep'ing for
specific uses difficult. Introduce the wrapper functions file_get_privdata
and file_set_privdata to help with the struct file uses.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Matthew Wilcox <matthew@wil.cx>

---

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9a96b4d..b357a17 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -961,6 +961,16 @@ extern spinlock_t files_lock;
 #define fput_atomic(x)	atomic_long_add_unless(&(x)->f_count, -1, 1)
 #define file_count(x)	atomic_long_read(&(x)->f_count)
 
+static inline void *file_get_privdata(struct file *file)
+{
+	return file->private_data;
+}
+
+static inline void file_set_privdata(struct file *file, void *data)
+{
+	file->private_data = data;
+}
+
 #ifdef CONFIG_DEBUG_WRITECOUNT
 static inline void file_take_write(struct file *f)
 {

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Christoph Hellwig]

On Mon, Aug 16, 2010 at 11:37:52AM -0700, H Hartley Sweeten wrote:
> The symbol 'private_data' is commonly used and makes grep'ing for
> specific uses difficult. Introduce the wrapper functions file_get_privdata
> and file_set_privdata to help with the struct file uses.

This just obsfucates the code, NAK.

RE: [PATCH] fs.h: introduce functions to get/set file->private_data

[H Hartley Sweeten]

On Monday, August 16, 2010 4:18 PM, Christoph Hellwig wrote:
> On Mon, Aug 16, 2010 at 11:37:52AM -0700, H Hartley Sweeten wrote:
>> The symbol 'private_data' is commonly used and makes grep'ing for
>> specific uses difficult. Introduce the wrapper functions file_get_privdata
>> and file_set_privdata to help with the struct file uses.
>
> This just obsfucates the code, NAK.

Not a problem.

It's just a pain trying to figure out where the file 'private_data' is being
used.

$ git grep private_data | wc -w
37272
$ git grep private_data include | wc -w
1068

The following struct's all have a 'void *private_data' symbol in include/linux:

struct ac97_codec
struct c2port_device
struct file
struct gendisk
struct ata_host
struct ata_queued_cmd
struct ata_device
struct ata_port
struct ata_port_info
struct parport
struct rchan
struct rtc_task
struct plat_serial8250_port
struct uart_port

include/rdma and include/sound also have a number.

Thanks for the reply.

Regards,
Hartley

RE: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Mon, 2010-08-16 at 18:36 -0500, H Hartley Sweeten wrote:
> It's just a pain trying to figure out where the file 'private_data' is being
> used.

spatch (coccinelle) could help a lot with this style problem.

For instance, here's a grep equivalent for any use
of (struct file) member private_data in directory fs

$ cat t.cocci
@@
struct file *file;
@@

* file->private_data
$ spatch -very_quiet -sp_file t.cocci fs

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Ted Ts'o]

And spatch could also be used to rename private_data to f_private_data
if people really cared....

					- Ted

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Mon, 2010-08-16 at 21:03 -0400, Ted Ts'o wrote:
> And spatch could also be used to rename private_data to f_private_data
> if people really cared....

In struct file, private_data is the only member not prefixed with f_.

It's a small .cocci file, but treewide, it's a relatively big patch.

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Christoph Hellwig]

On Mon, Aug 16, 2010 at 09:03:55PM -0400, Ted Ts'o wrote:
> And spatch could also be used to rename private_data to f_private_data
> if people really cared....

Agreed, that's much better than adding useless accessors.

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Tue, 2010-08-17 at 04:54 -0400, Christoph Hellwig wrote:
> On Mon, Aug 16, 2010 at 09:03:55PM -0400, Ted Ts'o wrote:
> > And spatch could also be used to rename private_data to f_private_data
> > if people really cared....
> Agreed, that's much better than adding useless accessors.

Against Linus' current, it's ~850KB.
Anyone really want it?

$ git diff --shortstat
 495 files changed, 2526 insertions(+), 2529 deletions(-)
$ git diff --stat -p > f_private_data.diff
$ ls -la f_private_data.diff
-rw-r--r-- 1 joe joe 853136 2010-08-17 10:19 f_private_data.diff

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Sam Ravnborg]

On Tue, Aug 17, 2010 at 10:26:13AM -0700, Joe Perches wrote:
> On Tue, 2010-08-17 at 04:54 -0400, Christoph Hellwig wrote:
> > On Mon, Aug 16, 2010 at 09:03:55PM -0400, Ted Ts'o wrote:
> > > And spatch could also be used to rename private_data to f_private_data
> > > if people really cared....
> > Agreed, that's much better than adding useless accessors.
> 
> Against Linus' current, it's ~850KB.
> Anyone really want it?

This clearly shows how big the effort would be to
introduce access functions.
And if the only gain is grepability then such a
cleanup patch is far more convinient.

It takes minimum effort to create and test, and if you can
get ack from Ted and/or Christoph there is a good chance Linus
would take it right before/after -rc1.

You obviously need to convince him that the patch has
seen decent build testing.

	Sam

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Tue, 2010-08-17 at 19:58 +0200, Sam Ravnborg wrote:
> On Tue, Aug 17, 2010 at 10:26:13AM -0700, Joe Perches wrote:
> > On Tue, 2010-08-17 at 04:54 -0400, Christoph Hellwig wrote:
> > > On Mon, Aug 16, 2010 at 09:03:55PM -0400, Ted Ts'o wrote:
> > > > And spatch could also be used to rename private_data to f_private_data
> > > > if people really cared....
> > > Agreed, that's much better than adding useless accessors.
> > Against Linus' current, it's ~850KB.
> > Anyone really want it?
> a cleanup patch is far more convinient.
> It takes minimum effort to create and test, and if you can
> get ack from Ted and/or Christoph there is a good chance Linus
> would take it right before/after -rc1.
> You obviously need to convince him that the patch has
> seen decent build testing.

I don't have the passel of cross compilers,
so someone else would need to do that.

I compiled it 32 bit x86 allyesconfig only.

Re: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Tue, 2010-08-17 at 19:58 +0200, Sam Ravnborg wrote:
> It takes minimum effort to create and test, and if you can
> get ack from Ted and/or Christoph there is a good chance Linus
> would take it right before/after -rc1.
> 
> You obviously need to convince him that the patch has
> seen decent build testing.

private_data is the only non f_ prefixed member in
struct file, so while there is perhaps a tiny value
in this patch, I'm not in a hurry to push it.

After a few corrections, Documentation/, comment and
printk updates, a few macros that spatch didn't update,
etc, the diff that converts struct file member
private_data to f_private_data is now:

$ git diff --shortstat ..da5cabf80e2433131bf0ed8993abc0f7ea618c73
 511 files changed, 2638 insertions(+), 2639 deletions(-)

Does either Ted or Christoph want to see it?

Harley?  You want it?

It's still ~850BKB and probably shouldn't be posted to
the ML as it's mostly mechanical.

Compiles x86 allyesconfig, defconfig

RE: [PATCH] fs.h: introduce functions to get/set file->private_data

[H Hartley Sweeten]

On Tuesday, August 17, 2010 6:32 PM, Joe Perches wrote:
> On Tue, 2010-08-17 at 19:58 +0200, Sam Ravnborg wrote:
>> It takes minimum effort to create and test, and if you can
>> get ack from Ted and/or Christoph there is a good chance Linus
>> would take it right before/after -rc1.
>> 
>> You obviously need to convince him that the patch has
>> seen decent build testing.
>
> private_data is the only non f_ prefixed member in
> struct file, so while there is perhaps a tiny value
> in this patch, I'm not in a hurry to push it.
>
> After a few corrections, Documentation/, comment and
> printk updates, a few macros that spatch didn't update,
> etc, the diff that converts struct file member
> private_data to f_private_data is now:
>
> $ git diff --shortstat ..da5cabf80e2433131bf0ed8993abc0f7ea618c73
>  511 files changed, 2638 insertions(+), 2639 deletions(-)
>
> Does either Ted or Christoph want to see it?
>
> Harley?  You want it?

Uh... No... ;-)

> It's still ~850BKB and probably shouldn't be posted to
> the ML as it's mostly mechanical.

I would like to see it merged just to ease grepping but if no one else
sees any benefit doing it I can live with it.

FWIW, the only reason for bringing this up in the first place was I was
trying to find all the places that have unnecessary casts when using
the private_data.  Stuff like:

	struct my_struct *my_data = (struct my_struct *)file->private_data;

Maybe it would be simpler to use spatch to just fix those?

> Compiles x86 allyesconfig, defconfig

Regards,
Hartley

RE: [PATCH] fs.h: introduce functions to get/set file->private_data

[Joe Perches]

On Wed, 2010-08-18 at 11:48 -0500, H Hartley Sweeten wrote:
> FWIW, the only reason for bringing this up in the first place was I was
> trying to find all the places that have unnecessary casts when using
> the private_data.  Stuff like:
> 
> 	struct my_struct *my_data = (struct my_struct *)file->private_data;
> 
> Maybe it would be simpler to use spatch to just fix those?

I submitted that patch last month.  About half have been applied.

(I generally link to lkml.org, but it seems down)
http://linux.derkeiler.com/Mailing-Lists/Kernel/2010-07/msg04333.html

I also delete the remaining casts in this conversion.