Skip to contentSkip to conversation

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

PATCH In review H Hartley Sweeten 5 participants 14 messages Aug 16, 2010 – Aug 18, 2010 VFS

lore
Share & tools
lore
#0 lore raw

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

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.

---

include/linux/fs.h +10 -0
index 9a96b4d..b357a17 100644
@@ -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)
 {
#1 lore raw
Quoted 3 lines · 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.

This just obsfucates the code, NAK.

#2 lore raw
Quoted 6 lines · Christoph Hellwig
Quoted 3 lines · 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.

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

#3 lore raw
Quoted 2 lines · H Hartley Sweeten

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
#4 lore raw

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

- Ted

#5 lore raw
Quoted 2 lines · Ted Ts'o

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.

#6 lore raw
In reply to Ted Ts'o
Quoted 2 lines · Ted Ts'o

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.

#7 lore raw
In reply to Ted Ts'o
Quoted 2 lines · Ted Ts'o

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.

#8 lore raw
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Ted Ts'o

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
#9 lore raw
Quoted 8 lines · Joe Perches
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Ted Ts'o

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

#10 lore raw
Quoted 6 lines · Sam Ravnborg
Quoted 5 lines · Joe Perches
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Ted Ts'o

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.

Quoted 8 lines
Quoted 2 lines

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.

#11 lore raw
In reply to Sam Ravnborg
Quoted 6 lines · Sam Ravnborg

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

#12 lore raw
Quoted 17 lines · Joe Perches
Quoted 6 lines · Sam Ravnborg

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:

Quoted 6 lines
$ 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... ;-)

Quoted 2 lines

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?

Quoted 1 line

Compiles x86 allyesconfig, defconfig

Regards,
Hartley

#13 lore raw
Quoted 7 lines · H Hartley Sweeten

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.