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 <[email protected]>
Cc: Matthew Wilcox <[email protected]>
---
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)
{
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.
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
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
And spatch could also be used to rename private_data to f_private_data
if people really cared....
- Ted
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.
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.
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
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
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.
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
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????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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.