fs.h: introduce functions to get/set file->private_data
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
@@ -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) {
Quoted 3 lines · H Hartley Sweeten
The symbol '
private_data' is commonly used and makes grep'ing for specific uses difficult. Introduce the wrapper functionsfile_get_privdataandfile_set_privdatato help with the struct file uses.
This just obsfucates the code, NAK.
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 functionsfile_get_privdataandfile_set_privdatato 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
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
And spatch could also be used to rename private_data to f_private_data if people really cared....
- Ted
Quoted 2 lines · Ted Ts'o
And spatch could also be used to rename
private_datatof_private_dataif 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.
Quoted 2 lines · Ted Ts'o
And spatch could also be used to rename
private_datatof_private_dataif people really cared....
Agreed, that's much better than adding useless accessors.
Quoted 2 lines · Ted Ts'o
And spatch could also be used to rename
private_datatof_private_dataif people really cared....
Agreed, that's much better than adding useless accessors.
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Ted Ts'o
And spatch could also be used to rename
private_datatof_private_dataif 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
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_datatof_private_dataif 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
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_datatof_private_dataif 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.
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
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_datais 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 memberprivate_datatof_private_datais 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
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.