[PATCH 0/2] Introduce simple_open()

[PATCH 0/2] Introduce simple_open()

[Stephen Boyd]

I noticed that if I want to tweak debugfs file ops I need to copy
the implementation of default_open() in fs/debugfs/file.c. I went
to look for an exported function that would be common across all
debugfs users but I couldn't find any.

This patchset moves default_open() to libfs so that other filesystems
can use it (although almost 99% of users are debugfs clients). I
wrote a semantic patch to find all the kernel users and ended up
with a nice diff:

 58 files changed, 150 insertions(+), 560 deletions(-)

I will send those patches as a follow up to these two once we settle
on naming and merging strategies for the patches.

Stephen Boyd (2):
  libfs: Add simple_open()
  coccinelle: semantic patch for simple_open()

 fs/libfs.c                               |    8 +++++++
 include/linux/fs.h                       |    1 +
 scripts/coccinelle/api/simple_open.cocci |   34 ++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 scripts/coccinelle/api/simple_open.cocci

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/2] libfs: Add simple_open()

[Stephen Boyd]

debugfs and a few other drivers use a version of simple_open() to
pass a pointer from the file to the read/write file ops. Add
support for this simple case to libfs so that we can remove the
many duplicate copies of this simple function.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 fs/libfs.c         |    8 ++++++++
 include/linux/fs.h |    1 +
 2 files changed, 9 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index 7c895a7..f5af1d6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -264,6 +264,13 @@ Enomem:
 	return ERR_PTR(-ENOMEM);
 }
 
+int simple_open(struct inode *inode, struct file *file)
+{
+	if (inode->i_private)
+		file->private_data = inode->i_private;
+	return 0;
+}
+
 int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = old_dentry->d_inode;
@@ -984,6 +991,7 @@ EXPORT_SYMBOL(simple_dir_operations);
 EXPORT_SYMBOL(simple_empty);
 EXPORT_SYMBOL(simple_fill_super);
 EXPORT_SYMBOL(simple_getattr);
+EXPORT_SYMBOL(simple_open);
 EXPORT_SYMBOL(simple_link);
 EXPORT_SYMBOL(simple_lookup);
 EXPORT_SYMBOL(simple_pin_fs);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f42a557..7871f90 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2517,6 +2517,7 @@ extern int dcache_readdir(struct file *, void *, filldir_t);
 extern int simple_setattr(struct dentry *, struct iattr *);
 extern int simple_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 extern int simple_statfs(struct dentry *, struct kstatfs *);
+extern int simple_open(struct inode *inode, struct file *file);
 extern int simple_link(struct dentry *, struct inode *, struct dentry *);
 extern int simple_unlink(struct inode *, struct dentry *);
 extern int simple_rmdir(struct inode *, struct dentry *);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/2] coccinelle: semantic patch for simple_open()

[Stephen Boyd]

Find instances of an open-coded simple_open() and replace them
with calls to simple_open().

Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

This seems to eat the definition of simple_open() as well. Is there
a way to say only remove both the function and the reference to
the function so that libfs survives?

 scripts/coccinelle/api/simple_open.cocci |   34 ++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 scripts/coccinelle/api/simple_open.cocci

diff --git a/scripts/coccinelle/api/simple_open.cocci b/scripts/coccinelle/api/simple_open.cocci
new file mode 100644
index 0000000..956d04d
--- /dev/null
+++ b/scripts/coccinelle/api/simple_open.cocci
@@ -0,0 +1,34 @@
+// This removes an open coded simple_open() function
+// and replaces file operations references to the function
+// with simple_open() instead.
+//
+
+virtual patch
+virtual context
+
+@ open @
+identifier open_f;
+identifier i, f;
+@@
+-int open_f(struct inode *i, struct file *f)
+-{
+(
+-if (i->i_private)
+-f->private_data = i->i_private;
+|
+-f->private_data = i->i_private;
+)
+-return 0;
+-}
+
+@ has_open depends on open @
+identifier fops;
+identifier open.open_f;
+@@
+struct file_operations fops = {
+...
+-.open = open_f,
++.open = simple_open,
+...
+};
+
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] coccinelle: semantic patch for simple_open()

[Julia Lawall]

On Sat, 3 Mar 2012, Stephen Boyd wrote:

> Find instances of an open-coded simple_open() and replace them
> with calls to simple_open().
>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>
> This seems to eat the definition of simple_open() as well. Is there
> a way to say only remove both the function and the reference to
> the function so that libfs survives?

Try identifier open_f != simple_open;

Is there a header file that should be present?

Your transformation rules should include depends on patch in the very 
first line, after the rule name.

Can you make a report rule?  If not, I will do it.  Someone asked for that 
to always be present.

thanks,
julia

> scripts/coccinelle/api/simple_open.cocci |   34 ++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 scripts/coccinelle/api/simple_open.cocci
>
> diff --git a/scripts/coccinelle/api/simple_open.cocci b/scripts/coccinelle/api/simple_open.cocci
> new file mode 100644
> index 0000000..956d04d
> --- /dev/null
> +++ b/scripts/coccinelle/api/simple_open.cocci
> @@ -0,0 +1,34 @@
> +// This removes an open coded simple_open() function
> +// and replaces file operations references to the function
> +// with simple_open() instead.
> +//
> +
> +virtual patch
> +virtual context
> +
> +@ open @
> +identifier open_f;
> +identifier i, f;
> +@@
> +-int open_f(struct inode *i, struct file *f)
> +-{
> +(
> +-if (i->i_private)
> +-f->private_data = i->i_private;
> +|
> +-f->private_data = i->i_private;
> +)
> +-return 0;
> +-}
> +
> +@ has_open depends on open @
> +identifier fops;
> +identifier open.open_f;
> +@@
> +struct file_operations fops = {
> +...
> +-.open = open_f,
> ++.open = simple_open,
> +...
> +};
> +
> -- 
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>

Re: [PATCH 2/2] coccinelle: semantic patch for simple_open()

[Stephen Boyd]

On 03/03/12 04:10, Julia Lawall wrote:
>
>
> On Sat, 3 Mar 2012, Stephen Boyd wrote:
>
>> Find instances of an open-coded simple_open() and replace them
>> with calls to simple_open().
>>
>> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>
>> This seems to eat the definition of simple_open() as well. Is there
>> a way to say only remove both the function and the reference to
>> the function so that libfs survives?
>
> Try identifier open_f != simple_open;

Ah thanks. That works.

>
> Is there a header file that should be present?

It would be <include/fs.h> which should already be there if there is a 
file_operations structure.

>
> Your transformation rules should include depends on patch in the very 
> first line, after the rule name.
>
> Can you make a report rule?  If not, I will do it.  Someone asked for 
> that to always be present.

I tried but failed. I get:

nit_defs_builtins: /usr/share/coccinelle/standard.h
between: p start 15 p end 15 m1 start 15 m1 end 15 m2 start 15 m2 end 15

   -int -open:open_f-struct -inode -*-open:i-, -struct -file -*-open:f
Fatal error: exception Failure("The semantic patch is structured in a 
way that may give bad results with isomorphisms.  Please try to rewrite 
it by moving + code out from -/context terms.")

When I add this:

@r1 depends on org || report @
identifier open_f != simple_open;
identifier i, f;
position p1;
@@
  int open_f@p1(struct inode *i, struct file *f)
  {
(
  if (i->i_private)
  f->private_data = i->i_private;
|
  f->private_data = i->i_private;
)
  return 0;
  }

@script:python depends on org@
p1 << r1.p1;
@@

cocci.print_main("extra function",p1)


Any ideas what's wrong?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.