Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-16 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
> This, BTW, is too convoluted for its own good.  What you need is
> something like
> struct whatever {
>   struct seq_file *m;
>   struct file *f;
>   int flags;
> };
> with single allocation of that sucker in your ->open().  Set
> file->private_data to address of seq_file field in your object *before*
> calling seq_open() and don't bother with m->private at all - just use
> container_of(m, struct whatever, m) in your ->show() to get to that
> structure...

Al, does the version below looks better? (Since it's harder to review diff,
here is the code after the patch applied).
---
struct proc_fdinfo {
struct seq_file m;
struct file *fd_file;
int f_flags;
};

static int seq_show(struct seq_file *m, void *v)
{
struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
   (long long)fdinfo->fd_file->f_pos, fdinfo->f_flags);
return 0;
}

static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo = NULL;
struct files_struct *files = NULL;
struct task_struct *task;
struct file *fd_file;
int f_flags, ret;

ret = -ENOENT;

task = get_proc_task(inode);
if (task) {
files = get_files_struct(task);
put_task_struct(task);
}

if (files) {
int fd = proc_fd(inode);

spin_lock(>file_lock);
fd_file = fcheck_files(files, fd);
if (fd_file) {
struct fdtable *fdt = files_fdtable(files);

f_flags = fd_file->f_flags & ~O_CLOEXEC;
if (close_on_exec(fd, fdt))
f_flags |= O_CLOEXEC;

get_file(fd_file);
ret = 0;
}
spin_unlock(>file_lock);

put_files_struct(files);
}

if (ret)
return ret;

ret = -ENOMEM;
fdinfo = kmalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
goto err_put;

fdinfo->fd_file = fd_file;
fdinfo->f_flags = f_flags;
file->private_data = >m;

ret = single_open(file, seq_show, NULL);
if (ret)
goto err_free;

return 0;

err_free:
kfree(fdinfo);
err_put:
fput(fd_file);
return ret;
}

static int seq_fdinfo_release(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo =
container_of((struct seq_file *)file->private_data,
 struct proc_fdinfo, m);
fput(fdinfo->fd_file);
return single_release(inode, file);
}

static const struct file_operations proc_fdinfo_file_operations = {
.open   = seq_fdinfo_open,
.read   = seq_read,
.llseek = seq_lseek,
.release= seq_fdinfo_release,
};
---
From: Cyrill Gorcunov 
Subject: procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2

This patch converts /proc/pid/fdinfo/ handling routines to seq-file which
is needed to extend seq operations and plug in auxiliary fdinfo provides
from subsystems like eventfd/eventpoll/fsnotify.

Note the proc_fd_link no longer call for proc_fd_info, simply because
proc_fd_info is converted to seq_fdinfo_open (which is seq-file open()
prototype).

v2 (by Al Viro):
 - Don't use helper function with optional arguments, thus proc_fd_info get 
deprecated
 - Use proc_fdinfo structure with seq_file embedded, thus we can use 
container_of helper
 - Use fput to free reference to the file we've grabbed

Signed-off-by: Cyrill Gorcunov 
CC: Pavel Emelyanov 
CC: Al Viro 
CC: Alexey Dobriyan 
CC: Andrew Morton 
CC: James Bottomley 
---
 fs/proc/fd.c |  145 ---
 1 file changed, 99 insertions(+), 46 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -6,61 +6,105 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include 
 
 #include "internal.h"
 #include "fd.h"
 
-#define PROC_FDINFO_MAX 64
+struct proc_fdinfo {
+   struct seq_file m;
+   struct file *fd_file;
+   int f_flags;
+};
+
+static int seq_show(struct seq_file *m, void *v)
+{
+   struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
+   seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
+  (long long)fdinfo->fd_file->f_pos, fdinfo->f_flags);
+   return 0;
+}
 
-static int proc_fd_info(struct inode *inode, struct path *path, char *info)
+static int seq_fdinfo_open(struct inode *inode, struct file

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-16 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
 This, BTW, is too convoluted for its own good.  What you need is
 something like
 struct whatever {
   struct seq_file *m;
   struct file *f;
   int flags;
 };
 with single allocation of that sucker in your -open().  Set
 file-private_data to address of seq_file field in your object *before*
 calling seq_open() and don't bother with m-private at all - just use
 container_of(m, struct whatever, m) in your -show() to get to that
 structure...

Al, does the version below looks better? (Since it's harder to review diff,
here is the code after the patch applied).
---
struct proc_fdinfo {
struct seq_file m;
struct file *fd_file;
int f_flags;
};

static int seq_show(struct seq_file *m, void *v)
{
struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
   (long long)fdinfo-fd_file-f_pos, fdinfo-f_flags);
return 0;
}

static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo = NULL;
struct files_struct *files = NULL;
struct task_struct *task;
struct file *fd_file;
int f_flags, ret;

ret = -ENOENT;

task = get_proc_task(inode);
if (task) {
files = get_files_struct(task);
put_task_struct(task);
}

if (files) {
int fd = proc_fd(inode);

spin_lock(files-file_lock);
fd_file = fcheck_files(files, fd);
if (fd_file) {
struct fdtable *fdt = files_fdtable(files);

f_flags = fd_file-f_flags  ~O_CLOEXEC;
if (close_on_exec(fd, fdt))
f_flags |= O_CLOEXEC;

get_file(fd_file);
ret = 0;
}
spin_unlock(files-file_lock);

put_files_struct(files);
}

if (ret)
return ret;

ret = -ENOMEM;
fdinfo = kmalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
goto err_put;

fdinfo-fd_file = fd_file;
fdinfo-f_flags = f_flags;
file-private_data = fdinfo-m;

ret = single_open(file, seq_show, NULL);
if (ret)
goto err_free;

return 0;

err_free:
kfree(fdinfo);
err_put:
fput(fd_file);
return ret;
}

static int seq_fdinfo_release(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo =
container_of((struct seq_file *)file-private_data,
 struct proc_fdinfo, m);
fput(fdinfo-fd_file);
return single_release(inode, file);
}

static const struct file_operations proc_fdinfo_file_operations = {
.open   = seq_fdinfo_open,
.read   = seq_read,
.llseek = seq_lseek,
.release= seq_fdinfo_release,
};
---
From: Cyrill Gorcunov gorcu...@openvz.org
Subject: procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2

This patch converts /proc/pid/fdinfo/ handling routines to seq-file which
is needed to extend seq operations and plug in auxiliary fdinfo provides
from subsystems like eventfd/eventpoll/fsnotify.

Note the proc_fd_link no longer call for proc_fd_info, simply because
proc_fd_info is converted to seq_fdinfo_open (which is seq-file open()
prototype).

v2 (by Al Viro):
 - Don't use helper function with optional arguments, thus proc_fd_info get 
deprecated
 - Use proc_fdinfo structure with seq_file embedded, thus we can use 
container_of helper
 - Use fput to free reference to the file we've grabbed

Signed-off-by: Cyrill Gorcunov gorcu...@openvz.org
CC: Pavel Emelyanov xe...@parallels.com
CC: Al Viro v...@zeniv.linux.org.uk
CC: Alexey Dobriyan adobri...@gmail.com
CC: Andrew Morton a...@linux-foundation.org
CC: James Bottomley jbottom...@parallels.com
---
 fs/proc/fd.c |  145 ---
 1 file changed, 99 insertions(+), 46 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -6,61 +6,105 @@
 #include linux/namei.h
 #include linux/pid.h
 #include linux/security.h
+#include linux/file.h
+#include linux/seq_file.h
 
 #include linux/proc_fs.h
 
 #include internal.h
 #include fd.h
 
-#define PROC_FDINFO_MAX 64
+struct proc_fdinfo {
+   struct seq_file m;
+   struct file *fd_file;
+   int f_flags;
+};
+
+static int seq_show(struct seq_file *m, void *v)
+{
+   struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
+   seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
+  (long

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
> 
> This, BTW, is too convoluted for its own good.  What you need is
> something like
> struct whatever {
>   struct seq_file *m;
>   struct file *f;
>   int flags;
> };
> with single allocation of that sucker in your ->open().  Set
> file->private_data to address of seq_file field in your object *before*
> calling seq_open() and don't bother with m->private at all - just use
> container_of(m, struct whatever, m) in your ->show() to get to that
> structure...

I will try and post results, thanks!

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:16:28PM +0100, Al Viro wrote:
> On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> > -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct 
> > path *path)
> > +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct 
> > file **f_file, struct path *path)
> 
> Bloody bad taste, that...  This kind of optional arguments is almost always
> a bad sign - tends to happen when you have two barely related functions
> crammed into one.  And yes, proc_fd_info() suffers the same braindamage.
> Trying to avoid code duplication is generally a good thing, but it's not
> always worth doing - less obfuscated code wins.

Sure I'll update. Thanks.

> >  static int seq_show(struct seq_file *m, void *v)
> >  {
> > struct proc_fdinfo *fdinfo = m->private;
> > -   seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> > -  (long long)fdinfo->f_pos,
> > -  fdinfo->f_flags);
> > -   return 0;
> > +   int ret;
> > +
> > +   ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> > +(long long)fdinfo->f_file->f_pos,
> > +fdinfo->f_flags);
> 
> Realistically, that one is not going to overflow; you are almost certainly
> wasting more cycles on that check of !ret just below than you'll save on
> not going into ->show_fdinfo() in case of full buffer.

Yes, this is redundant, thanks. Will fix.

> 
> > +   if (!ret && fdinfo->f_file->f_op->show_fdinfo)
> > +   ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
> > +
> > +   return ret;
> >  }
>   
> > +   ret = single_open(file, seq_show, fdinfo);
> > +   if (ret) {
> > +   put_filp(fdinfo->f_file);
> 
> Excuse me?  We should *never* do put_filp() on anything that has already
> been opened.  Think what happens if you race with close(); close() would
> rip the reference from descriptor table and do fput(), leaving you with
> the last reference to that struct file.  You really don't want to just
> go and free it.  IOW, that one should be fput().
> 
> > +   put_filp(fdinfo->f_file);
> Ditto. 

It seems I indeed missed this scenario, thanks Al, will update!

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Al Viro 
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
>  struct proc_fdinfo {
> - loff_t  f_pos;
> - int f_flags;
> + struct file *f_file;
> + int f_flags;
>  };

> + struct proc_fdinfo *fdinfo;
> + struct seq_file *m;
> + int ret;
>  
>   fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
>   if (!fdinfo)
>   return -ENOMEM;

> + ret = single_open(file, seq_show, fdinfo);
> + if (ret) {
> + put_filp(fdinfo->f_file);
> + goto err_free;
>   }
>  
> + m = file->private_data;
> + m->private = fdinfo;

This, BTW, is too convoluted for its own good.  What you need is
something like
struct whatever {
struct seq_file *m;
struct file *f;
int flags;
};
with single allocation of that sucker in your ->open().  Set
file->private_data to address of seq_file field in your object *before*
calling seq_open() and don't bother with m->private at all - just use
container_of(m, struct whatever, m) in your ->show() to get to that
structure...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Al Viro 
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
> *path)
> +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
> **f_file, struct path *path)

Bloody bad taste, that...  This kind of optional arguments is almost always
a bad sign - tends to happen when you have two barely related functions
crammed into one.  And yes, proc_fd_info() suffers the same braindamage.
Trying to avoid code duplication is generally a good thing, but it's not
always worth doing - less obfuscated code wins.

>  static int seq_show(struct seq_file *m, void *v)
>  {
>   struct proc_fdinfo *fdinfo = m->private;
> - seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> -(long long)fdinfo->f_pos,
> -fdinfo->f_flags);
> - return 0;
> + int ret;
> +
> + ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> +  (long long)fdinfo->f_file->f_pos,
> +  fdinfo->f_flags);

Realistically, that one is not going to overflow; you are almost certainly
wasting more cycles on that check of !ret just below than you'll save on
not going into ->show_fdinfo() in case of full buffer.

> + if (!ret && fdinfo->f_file->f_op->show_fdinfo)
> + ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
> +
> + return ret;
>  }
  
> + ret = single_open(file, seq_show, fdinfo);
> + if (ret) {
> + put_filp(fdinfo->f_file);

Excuse me?  We should *never* do put_filp() on anything that has already
been opened.  Think what happens if you race with close(); close() would
rip the reference from descriptor table and do fput(), leaving you with
the last reference to that struct file.  You really don't want to just
go and free it.  IOW, that one should be fput().

> + put_filp(fdinfo->f_file);
Ditto.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 01:07:03AM +0100, Al Viro wrote:
> On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
> > > Hmm, in very first versions I've been using one ->show method, but
> > > then I thought that this is not very correlate with seq-files idea
> > > where for each record show/next sequence is called. I'll update (this
> > > for sure will make code simplier, and I'll have to check for seq-file
> > > overflow after seq_printf call to not continue printing data for too
> > > long if buffer already out of space).
> > 
> > Al, I'll cook the whole series tomorrow and resend it for review,
> > also I guess the new show_fdinfo() member in file-operations should
> > be guarded with CONFIG_PROC_FS, right?
> 
> I seriously doubt that it's worth bothering.  If somebody cares, they
> can add making it conditional later.

That's what I've beed testing, does it looks good for you?
---
From: Cyrill Gorcunov 
Subject: procfs: Add ability to plug in auxiliary fdinfo providers

This patch brings ability to print out auxiliary data associated
with file in procfs interface /proc/pid/fdinfo/fd.

Inparticular further patches make eventfd, evenpoll, signalfd
and fsnotify to print additional information complete enough
to restore these objects after checkpoint.

To simplify the code we add show_fdinfo callback into
struct file_operations (as Al proposed).

Signed-off-by: Cyrill Gorcunov 
CC: Pavel Emelyanov 
CC: Al Viro 
CC: Alexey Dobriyan 
CC: Andrew Morton 
CC: James Bottomley 
---
 fs/proc/fd.c   |   51 ---
 include/linux/fs.h |3 +++
 2 files changed, 39 insertions(+), 15 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -15,11 +15,11 @@
 #include "fd.h"
 
 struct proc_fdinfo {
-   loff_t  f_pos;
-   int f_flags;
+   struct file *f_file;
+   int f_flags;
 };
 
-static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
*path)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
**f_file, struct path *path)
 {
struct files_struct *files = NULL;
struct task_struct *task;
@@ -49,6 +49,10 @@ static int fdinfo_open_helper(struct ino
*path = fd_file->f_path;
path_get(_file->f_path);
}
+   if (f_file) {
+   *f_file = fd_file;
+   get_file(fd_file);
+   }
ret = 0;
}
spin_unlock(>file_lock);
@@ -61,28 +65,44 @@ static int fdinfo_open_helper(struct ino
 static int seq_show(struct seq_file *m, void *v)
 {
struct proc_fdinfo *fdinfo = m->private;
-   seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
-  (long long)fdinfo->f_pos,
-  fdinfo->f_flags);
-   return 0;
+   int ret;
+
+   ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
+(long long)fdinfo->f_file->f_pos,
+fdinfo->f_flags);
+
+   if (!ret && fdinfo->f_file->f_op->show_fdinfo)
+   ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
+
+   return ret;
 }
 
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
 {
-   struct proc_fdinfo *fdinfo = NULL;
-   int ret = -ENOENT;
+   struct proc_fdinfo *fdinfo;
+   struct seq_file *m;
+   int ret;
 
fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
return -ENOMEM;
 
-   ret = fdinfo_open_helper(inode, >f_flags, NULL);
-   if (!ret) {
-   ret = single_open(file, seq_show, fdinfo);
-   if (!ret)
-   fdinfo = NULL;
+   ret = fdinfo_open_helper(inode, >f_flags, >f_file, 
NULL);
+   if (ret)
+   goto err_free;
+
+   ret = single_open(file, seq_show, fdinfo);
+   if (ret) {
+   put_filp(fdinfo->f_file);
+   goto err_free;
}
 
+   m = file->private_data;
+   m->private = fdinfo;
+
+   return ret;
+
+err_free:
kfree(fdinfo);
return ret;
 }
@@ -92,6 +112,7 @@ static int seq_fdinfo_release(struct ino
struct seq_file *m = file->private_data;
struct proc_fdinfo *fdinfo = m->private;
 
+   put_filp(fdinfo->f_file);
kfree(fdinfo);
 
return single_release(inode, file);
@@ -173,7 +194,7 @@ static const struct dentry_operations ti
 
 static int proc_fd_link(struct dentry *dentry, struct path *path)
 {
-   return fdinfo_open_helper(dentry->d_inode, NULL, path);
+   return fdinfo_open_helper(dentry->d_inode, NULL, NULL, path);
 }
 
 static struct dentry *
Index: linux-2.6.git/include/linux/fs.h

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 01:07:03AM +0100, Al Viro wrote:
 On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
   Hmm, in very first versions I've been using one -show method, but
   then I thought that this is not very correlate with seq-files idea
   where for each record show/next sequence is called. I'll update (this
   for sure will make code simplier, and I'll have to check for seq-file
   overflow after seq_printf call to not continue printing data for too
   long if buffer already out of space).
  
  Al, I'll cook the whole series tomorrow and resend it for review,
  also I guess the new show_fdinfo() member in file-operations should
  be guarded with CONFIG_PROC_FS, right?
 
 I seriously doubt that it's worth bothering.  If somebody cares, they
 can add making it conditional later.

That's what I've beed testing, does it looks good for you?
---
From: Cyrill Gorcunov gorcu...@openvz.org
Subject: procfs: Add ability to plug in auxiliary fdinfo providers

This patch brings ability to print out auxiliary data associated
with file in procfs interface /proc/pid/fdinfo/fd.

Inparticular further patches make eventfd, evenpoll, signalfd
and fsnotify to print additional information complete enough
to restore these objects after checkpoint.

To simplify the code we add show_fdinfo callback into
struct file_operations (as Al proposed).

Signed-off-by: Cyrill Gorcunov gorcu...@openvz.org
CC: Pavel Emelyanov xe...@parallels.com
CC: Al Viro v...@zeniv.linux.org.uk
CC: Alexey Dobriyan adobri...@gmail.com
CC: Andrew Morton a...@linux-foundation.org
CC: James Bottomley jbottom...@parallels.com
---
 fs/proc/fd.c   |   51 ---
 include/linux/fs.h |3 +++
 2 files changed, 39 insertions(+), 15 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -15,11 +15,11 @@
 #include fd.h
 
 struct proc_fdinfo {
-   loff_t  f_pos;
-   int f_flags;
+   struct file *f_file;
+   int f_flags;
 };
 
-static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
*path)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
**f_file, struct path *path)
 {
struct files_struct *files = NULL;
struct task_struct *task;
@@ -49,6 +49,10 @@ static int fdinfo_open_helper(struct ino
*path = fd_file-f_path;
path_get(fd_file-f_path);
}
+   if (f_file) {
+   *f_file = fd_file;
+   get_file(fd_file);
+   }
ret = 0;
}
spin_unlock(files-file_lock);
@@ -61,28 +65,44 @@ static int fdinfo_open_helper(struct ino
 static int seq_show(struct seq_file *m, void *v)
 {
struct proc_fdinfo *fdinfo = m-private;
-   seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
-  (long long)fdinfo-f_pos,
-  fdinfo-f_flags);
-   return 0;
+   int ret;
+
+   ret = seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
+(long long)fdinfo-f_file-f_pos,
+fdinfo-f_flags);
+
+   if (!ret  fdinfo-f_file-f_op-show_fdinfo)
+   ret = fdinfo-f_file-f_op-show_fdinfo(m, fdinfo-f_file);
+
+   return ret;
 }
 
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
 {
-   struct proc_fdinfo *fdinfo = NULL;
-   int ret = -ENOENT;
+   struct proc_fdinfo *fdinfo;
+   struct seq_file *m;
+   int ret;
 
fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
return -ENOMEM;
 
-   ret = fdinfo_open_helper(inode, fdinfo-f_flags, NULL);
-   if (!ret) {
-   ret = single_open(file, seq_show, fdinfo);
-   if (!ret)
-   fdinfo = NULL;
+   ret = fdinfo_open_helper(inode, fdinfo-f_flags, fdinfo-f_file, 
NULL);
+   if (ret)
+   goto err_free;
+
+   ret = single_open(file, seq_show, fdinfo);
+   if (ret) {
+   put_filp(fdinfo-f_file);
+   goto err_free;
}
 
+   m = file-private_data;
+   m-private = fdinfo;
+
+   return ret;
+
+err_free:
kfree(fdinfo);
return ret;
 }
@@ -92,6 +112,7 @@ static int seq_fdinfo_release(struct ino
struct seq_file *m = file-private_data;
struct proc_fdinfo *fdinfo = m-private;
 
+   put_filp(fdinfo-f_file);
kfree(fdinfo);
 
return single_release(inode, file);
@@ -173,7 +194,7 @@ static const struct dentry_operations ti
 
 static int proc_fd_link(struct dentry *dentry, struct path *path)
 {
-   return fdinfo_open_helper(dentry-d_inode, NULL, path);
+   return fdinfo_open_helper(dentry-d_inode, NULL, NULL, path);

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Al Viro 
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
 -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
 *path)
 +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
 **f_file, struct path *path)

Bloody bad taste, that...  This kind of optional arguments is almost always
a bad sign - tends to happen when you have two barely related functions
crammed into one.  And yes, proc_fd_info() suffers the same braindamage.
Trying to avoid code duplication is generally a good thing, but it's not
always worth doing - less obfuscated code wins.

  static int seq_show(struct seq_file *m, void *v)
  {
   struct proc_fdinfo *fdinfo = m-private;
 - seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
 -(long long)fdinfo-f_pos,
 -fdinfo-f_flags);
 - return 0;
 + int ret;
 +
 + ret = seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
 +  (long long)fdinfo-f_file-f_pos,
 +  fdinfo-f_flags);

Realistically, that one is not going to overflow; you are almost certainly
wasting more cycles on that check of !ret just below than you'll save on
not going into -show_fdinfo() in case of full buffer.

 + if (!ret  fdinfo-f_file-f_op-show_fdinfo)
 + ret = fdinfo-f_file-f_op-show_fdinfo(m, fdinfo-f_file);
 +
 + return ret;
  }
  
 + ret = single_open(file, seq_show, fdinfo);
 + if (ret) {
 + put_filp(fdinfo-f_file);

Excuse me?  We should *never* do put_filp() on anything that has already
been opened.  Think what happens if you race with close(); close() would
rip the reference from descriptor table and do fput(), leaving you with
the last reference to that struct file.  You really don't want to just
go and free it.  IOW, that one should be fput().

 + put_filp(fdinfo-f_file);
Ditto.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Al Viro 
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
  struct proc_fdinfo {
 - loff_t  f_pos;
 - int f_flags;
 + struct file *f_file;
 + int f_flags;
  };

 + struct proc_fdinfo *fdinfo;
 + struct seq_file *m;
 + int ret;
  
   fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
   if (!fdinfo)
   return -ENOMEM;

 + ret = single_open(file, seq_show, fdinfo);
 + if (ret) {
 + put_filp(fdinfo-f_file);
 + goto err_free;
   }
  
 + m = file-private_data;
 + m-private = fdinfo;

This, BTW, is too convoluted for its own good.  What you need is
something like
struct whatever {
struct seq_file *m;
struct file *f;
int flags;
};
with single allocation of that sucker in your -open().  Set
file-private_data to address of seq_file field in your object *before*
calling seq_open() and don't bother with m-private at all - just use
container_of(m, struct whatever, m) in your -show() to get to that
structure...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:16:28PM +0100, Al Viro wrote:
 On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
  -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct 
  path *path)
  +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct 
  file **f_file, struct path *path)
 
 Bloody bad taste, that...  This kind of optional arguments is almost always
 a bad sign - tends to happen when you have two barely related functions
 crammed into one.  And yes, proc_fd_info() suffers the same braindamage.
 Trying to avoid code duplication is generally a good thing, but it's not
 always worth doing - less obfuscated code wins.

Sure I'll update. Thanks.

   static int seq_show(struct seq_file *m, void *v)
   {
  struct proc_fdinfo *fdinfo = m-private;
  -   seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
  -  (long long)fdinfo-f_pos,
  -  fdinfo-f_flags);
  -   return 0;
  +   int ret;
  +
  +   ret = seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
  +(long long)fdinfo-f_file-f_pos,
  +fdinfo-f_flags);
 
 Realistically, that one is not going to overflow; you are almost certainly
 wasting more cycles on that check of !ret just below than you'll save on
 not going into -show_fdinfo() in case of full buffer.

Yes, this is redundant, thanks. Will fix.

 
  +   if (!ret  fdinfo-f_file-f_op-show_fdinfo)
  +   ret = fdinfo-f_file-f_op-show_fdinfo(m, fdinfo-f_file);
  +
  +   return ret;
   }
   
  +   ret = single_open(file, seq_show, fdinfo);
  +   if (ret) {
  +   put_filp(fdinfo-f_file);
 
 Excuse me?  We should *never* do put_filp() on anything that has already
 been opened.  Think what happens if you race with close(); close() would
 rip the reference from descriptor table and do fput(), leaving you with
 the last reference to that struct file.  You really don't want to just
 go and free it.  IOW, that one should be fput().
 
  +   put_filp(fdinfo-f_file);
 Ditto. 

It seems I indeed missed this scenario, thanks Al, will update!

Cyrill
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-15 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
 
 This, BTW, is too convoluted for its own good.  What you need is
 something like
 struct whatever {
   struct seq_file *m;
   struct file *f;
   int flags;
 };
 with single allocation of that sucker in your -open().  Set
 file-private_data to address of seq_file field in your object *before*
 calling seq_open() and don't bother with m-private at all - just use
 container_of(m, struct whatever, m) in your -show() to get to that
 structure...

I will try and post results, thanks!

Cyrill
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
> > Hmm, in very first versions I've been using one ->show method, but
> > then I thought that this is not very correlate with seq-files idea
> > where for each record show/next sequence is called. I'll update (this
> > for sure will make code simplier, and I'll have to check for seq-file
> > overflow after seq_printf call to not continue printing data for too
> > long if buffer already out of space).
> 
> Al, I'll cook the whole series tomorrow and resend it for review,
> also I guess the new show_fdinfo() member in file-operations should
> be guarded with CONFIG_PROC_FS, right?

I seriously doubt that it's worth bothering.  If somebody cares, they
can add making it conditional later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 01:56:16AM +0400, Cyrill Gorcunov wrote:
> > >  struct file_operations {
> > >   struct module *owner;
> > > + struct seq_operations *fdinfo_ops;
> > 
> > IDGI.  Why on the earth do you need the whole iterator?  All it takes
> > is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
> > your iterator (one going through the files) call that sucker for the
> > file we are trying to show.
> > 
> > I think you are severely overdesigning that thing...
> 
> Hmm, in very first versions I've been using one ->show method, but
> then I thought that this is not very correlate with seq-files idea
> where for each record show/next sequence is called. I'll update (this
> for sure will make code simplier, and I'll have to check for seq-file
> overflow after seq_printf call to not continue printing data for too
> long if buffer already out of space).

Al, I'll cook the whole series tomorrow and resend it for review,
also I guess the new show_fdinfo() member in file-operations should
be guarded with CONFIG_PROC_FS, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 10:27:21PM +0100, Al Viro wrote:
> On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
> > Al, does the patch below looks better? If so I'll fix up the rest.
> 
> >  struct file_operations {
> > struct module *owner;
> > +   struct seq_operations *fdinfo_ops;
> 
> IDGI.  Why on the earth do you need the whole iterator?  All it takes
> is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
> your iterator (one going through the files) call that sucker for the
> file we are trying to show.
> 
> I think you are severely overdesigning that thing...

Hmm, in very first versions I've been using one ->show method, but
then I thought that this is not very correlate with seq-files idea
where for each record show/next sequence is called. I'll update (this
for sure will make code simplier, and I'll have to check for seq-file
overflow after seq_printf call to not continue printing data for too
long if buffer already out of space).

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
> Al, does the patch below looks better? If so I'll fix up the rest.

>  struct file_operations {
>   struct module *owner;
> + struct seq_operations *fdinfo_ops;

IDGI.  Why on the earth do you need the whole iterator?  All it takes
is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
your iterator (one going through the files) call that sucker for the
file we are trying to show.

I think you are severely overdesigning that thing...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 10:35:58PM +0400, Cyrill Gorcunov wrote:
> On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
> > > Initially we considered to inject some "show" metod to
> > > file_operations but since there really a number of
> > > file_operations declared inside kernel (and in real the
> > > further patches cover onle eventfd/epoll/inotify) the
> > > waste of memory space will be inacceptable I think.
> > 
> > NAK.  This is too ugly to live.  Put it into file_operations,
> > with NULL meaning default output, or don't do it at all.
> > 
> > And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
> > fly - we have all seen it played too many times.  All it takes
> > is one politically-inclined induhvidual adding a dependency to
> > some "vertically integrated" turd (*cough* systemd *spit* udev
> > *cough*) and we are stuck with the damn thing.  CGROUP shite
> > is already there, DEVTMPFS is well on its way, etc.
> 
> OK, I'll put it into file_operations then (actually Pavel was
> proposing the same but I've been scared by amount of file_operations
> declared). Thanks!

Al, does the patch below looks better? If so I'll fix up the rest.
---
 fs/proc/fd.c|  109 ++--
 include/linux/fs.h  |3 +
 include/linux/proc_fs.h |8 +++
 3 files changed, 99 insertions(+), 21 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -8,18 +8,14 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
 #include "internal.h"
 #include "fd.h"
 
-struct proc_fdinfo {
-   loff_t  f_pos;
-   int f_flags;
-};
-
-static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
*path)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
**f_file, struct path *path)
 {
struct files_struct *files = NULL;
struct task_struct *task;
@@ -49,6 +45,10 @@ static int fdinfo_open_helper(struct ino
*path = fd_file->f_path;
path_get(_file->f_path);
}
+   if (f_file) {
+   *f_file = fd_file;
+   get_file(fd_file);
+   }
ret = 0;
}
spin_unlock(>file_lock);
@@ -58,43 +58,110 @@ static int fdinfo_open_helper(struct ino
return ret;
 }
 
+static void *seq_start(struct seq_file *m, loff_t *pos)
+{
+   struct proc_fdinfo_extra *extra = m->private;
+
+   extra->pos = *pos;
+
+   return *pos == 0 ? extra :
+   (extra->fdinfo_ops ?
+extra->fdinfo_ops->start(m, pos) : NULL);
+}
+
+static void seq_stop(struct seq_file *m, void *v)
+{
+   struct proc_fdinfo_extra *extra = m->private;
+
+   if (extra->fdinfo_ops && extra->pos > 0)
+   extra->fdinfo_ops->stop(m, v);
+}
+
+static void *seq_next(struct seq_file *m, void *p, loff_t *pos)
+{
+   struct proc_fdinfo_extra *extra = m->private;
+   void *v = NULL;
+
+   if (extra->fdinfo_ops) {
+   int ret = 0;
+
+   if (*pos == 0) {
+   v = extra->fdinfo_ops->start(m, pos);
+   if (v) {
+   ret = extra->fdinfo_ops->show(m, v);
+   p = v;
+   } else
+   ret = -1;
+   }
+
+   if (!ret)
+   v = extra->fdinfo_ops->next(m, p, pos);
+   } else
+   ++*pos;
+
+   extra->pos = *pos;
+   return v;
+}
+
 static int seq_show(struct seq_file *m, void *v)
 {
-   struct proc_fdinfo *fdinfo = m->private;
+   struct proc_fdinfo_extra *extra = m->private;
+
+   if (extra->fdinfo_ops && extra->pos > 0)
+   return extra->fdinfo_ops->show(m, v);
+
seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
-  (long long)fdinfo->f_pos,
-  fdinfo->f_flags);
+  (long long)extra->f_file->f_pos,
+  extra->f_flags);
return 0;
 }
 
+static const struct seq_operations fdinfo_seq_ops = {
+   .start  = seq_start,
+   .next   = seq_next,
+   .stop   = seq_stop,
+   .show   = seq_show,
+};
+
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
 {
-   struct proc_fdinfo *fdinfo = NULL;
-   int ret = -ENOENT;
+   struct proc_fdinfo_extra *extra;
+   struct seq_file *m;
+   int ret;
 
-   fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
-   if (!fdinfo)
+   extra = kzalloc(sizeof(*extra), GFP_KERNEL);
+   if (!extra)
return -ENOMEM;
 
-   ret = fdinfo_open_helper(inode, >f_flags, NULL);
+   ret = seq_open(file, _seq_ops);
if (!ret) {
-

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
> > Initially we considered to inject some "show" metod to
> > file_operations but since there really a number of
> > file_operations declared inside kernel (and in real the
> > further patches cover onle eventfd/epoll/inotify) the
> > waste of memory space will be inacceptable I think.
> 
> NAK.  This is too ugly to live.  Put it into file_operations,
> with NULL meaning default output, or don't do it at all.
> 
> And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
> fly - we have all seen it played too many times.  All it takes
> is one politically-inclined induhvidual adding a dependency to
> some "vertically integrated" turd (*cough* systemd *spit* udev
> *cough*) and we are stuck with the damn thing.  CGROUP shite
> is already there, DEVTMPFS is well on its way, etc.

OK, I'll put it into file_operations then (actually Pavel was
proposing the same but I've been scared by amount of file_operations
declared). Thanks!

Cyrill
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Tue, Aug 14, 2012 at 06:03:45PM +0400, Cyrill Gorcunov wrote:
> This patch brings ability to plug in auxiliary fdinfo providers.
> For example in further patches eventfd, evenpoll and fsnotify
> will print out information associated with files.
> 
> This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
> overhead for those who don't need it at all (this
> unfortunately makes patch bigger than I wanted).
> 
> The basic usage rule is the following
> 
>  - fdinfo provider should register own "show" method
>via proc_register_fdinfo_driver call, where "show"
>methods are rather well known seq-file operations
> 
>  - once the kernel opens /proc/$pid/fdinfo/$fd file
>it calls for ->probe() method in registered fdinfo
>drivers, and if probe success, then seq-file "show"
>operations will be called to provide out additional
>infomation
> 
> Initially we considered to inject some "show" metod to
> file_operations but since there really a number of
> file_operations declared inside kernel (and in real the
> further patches cover onle eventfd/epoll/inotify) the
> waste of memory space will be inacceptable I think.

NAK.  This is too ugly to live.  Put it into file_operations,
with NULL meaning default output, or don't do it at all.

And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
fly - we have all seen it played too many times.  All it takes
is one politically-inclined induhvidual adding a dependency to
some "vertically integrated" turd (*cough* systemd *spit* udev
*cough*) and we are stuck with the damn thing.  CGROUP shite
is already there, DEVTMPFS is well on its way, etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Pavel Emelyanov 
On 08/14/2012 06:03 PM, Cyrill Gorcunov wrote:
> This patch brings ability to plug in auxiliary fdinfo providers.
> For example in further patches eventfd, evenpoll and fsnotify
> will print out information associated with files.
> 
> This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
> overhead for those who don't need it at all (this
> unfortunately makes patch bigger than I wanted).
> 
> The basic usage rule is the following
> 
>  - fdinfo provider should register own "show" method
>via proc_register_fdinfo_driver call, where "show"
>methods are rather well known seq-file operations
> 
>  - once the kernel opens /proc/$pid/fdinfo/$fd file
>it calls for ->probe() method in registered fdinfo
>drivers, and if probe success, then seq-file "show"
>operations will be called to provide out additional
>infomation
> 
> Initially we considered to inject some "show" metod to
> file_operations but since there really a number of
> file_operations declared inside kernel (and in real the
> further patches cover onle eventfd/epoll/inotify) the
> waste of memory space will be inacceptable I think.
> 
> Pavel, I've left seq_next memthod as it was simply because
> we can't leave seq_next() after calling extra->driver->ops->start
> without increasing "pos", thus we need to call for "show" manually
> once.
> 
> Signed-off-by: Cyrill Gorcunov 
> CC: Al Viro 
> CC: Alexey Dobriyan 
> CC: Andrew Morton 
> CC: Pavel Emelyanov 
> CC: James Bottomley 

Acked-by: Pavel Emelyanov 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Pavel Emelyanov 
On 08/14/2012 06:03 PM, Cyrill Gorcunov wrote:
 This patch brings ability to plug in auxiliary fdinfo providers.
 For example in further patches eventfd, evenpoll and fsnotify
 will print out information associated with files.
 
 This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
 overhead for those who don't need it at all (this
 unfortunately makes patch bigger than I wanted).
 
 The basic usage rule is the following
 
  - fdinfo provider should register own show method
via proc_register_fdinfo_driver call, where show
methods are rather well known seq-file operations
 
  - once the kernel opens /proc/$pid/fdinfo/$fd file
it calls for -probe() method in registered fdinfo
drivers, and if probe success, then seq-file show
operations will be called to provide out additional
infomation
 
 Initially we considered to inject some show metod to
 file_operations but since there really a number of
 file_operations declared inside kernel (and in real the
 further patches cover onle eventfd/epoll/inotify) the
 waste of memory space will be inacceptable I think.
 
 Pavel, I've left seq_next memthod as it was simply because
 we can't leave seq_next() after calling extra-driver-ops-start
 without increasing pos, thus we need to call for show manually
 once.
 
 Signed-off-by: Cyrill Gorcunov gorcu...@openvz.org
 CC: Al Viro v...@zeniv.linux.org.uk
 CC: Alexey Dobriyan adobri...@gmail.com
 CC: Andrew Morton a...@linux-foundation.org
 CC: Pavel Emelyanov xe...@parallels.com
 CC: James Bottomley jbottom...@parallels.com

Acked-by: Pavel Emelyanov xe...@parallels.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Tue, Aug 14, 2012 at 06:03:45PM +0400, Cyrill Gorcunov wrote:
 This patch brings ability to plug in auxiliary fdinfo providers.
 For example in further patches eventfd, evenpoll and fsnotify
 will print out information associated with files.
 
 This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
 overhead for those who don't need it at all (this
 unfortunately makes patch bigger than I wanted).
 
 The basic usage rule is the following
 
  - fdinfo provider should register own show method
via proc_register_fdinfo_driver call, where show
methods are rather well known seq-file operations
 
  - once the kernel opens /proc/$pid/fdinfo/$fd file
it calls for -probe() method in registered fdinfo
drivers, and if probe success, then seq-file show
operations will be called to provide out additional
infomation
 
 Initially we considered to inject some show metod to
 file_operations but since there really a number of
 file_operations declared inside kernel (and in real the
 further patches cover onle eventfd/epoll/inotify) the
 waste of memory space will be inacceptable I think.

NAK.  This is too ugly to live.  Put it into file_operations,
with NULL meaning default output, or don't do it at all.

And no, it's only if you enable CONFIG_SOME_SHIT gambit won't
fly - we have all seen it played too many times.  All it takes
is one politically-inclined induhvidual adding a dependency to
some vertically integrated turd (*cough* systemd *spit* udev
*cough*) and we are stuck with the damn thing.  CGROUP shite
is already there, DEVTMPFS is well on its way, etc.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
  Initially we considered to inject some show metod to
  file_operations but since there really a number of
  file_operations declared inside kernel (and in real the
  further patches cover onle eventfd/epoll/inotify) the
  waste of memory space will be inacceptable I think.
 
 NAK.  This is too ugly to live.  Put it into file_operations,
 with NULL meaning default output, or don't do it at all.
 
 And no, it's only if you enable CONFIG_SOME_SHIT gambit won't
 fly - we have all seen it played too many times.  All it takes
 is one politically-inclined induhvidual adding a dependency to
 some vertically integrated turd (*cough* systemd *spit* udev
 *cough*) and we are stuck with the damn thing.  CGROUP shite
 is already there, DEVTMPFS is well on its way, etc.

OK, I'll put it into file_operations then (actually Pavel was
proposing the same but I've been scared by amount of file_operations
declared). Thanks!

Cyrill
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 10:35:58PM +0400, Cyrill Gorcunov wrote:
 On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
   Initially we considered to inject some show metod to
   file_operations but since there really a number of
   file_operations declared inside kernel (and in real the
   further patches cover onle eventfd/epoll/inotify) the
   waste of memory space will be inacceptable I think.
  
  NAK.  This is too ugly to live.  Put it into file_operations,
  with NULL meaning default output, or don't do it at all.
  
  And no, it's only if you enable CONFIG_SOME_SHIT gambit won't
  fly - we have all seen it played too many times.  All it takes
  is one politically-inclined induhvidual adding a dependency to
  some vertically integrated turd (*cough* systemd *spit* udev
  *cough*) and we are stuck with the damn thing.  CGROUP shite
  is already there, DEVTMPFS is well on its way, etc.
 
 OK, I'll put it into file_operations then (actually Pavel was
 proposing the same but I've been scared by amount of file_operations
 declared). Thanks!

Al, does the patch below looks better? If so I'll fix up the rest.
---
 fs/proc/fd.c|  109 ++--
 include/linux/fs.h  |3 +
 include/linux/proc_fs.h |8 +++
 3 files changed, 99 insertions(+), 21 deletions(-)

Index: linux-2.6.git/fs/proc/fd.c
===
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -8,18 +8,14 @@
 #include linux/security.h
 #include linux/file.h
 #include linux/seq_file.h
+#include linux/spinlock.h
 
 #include linux/proc_fs.h
 
 #include internal.h
 #include fd.h
 
-struct proc_fdinfo {
-   loff_t  f_pos;
-   int f_flags;
-};
-
-static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path 
*path)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file 
**f_file, struct path *path)
 {
struct files_struct *files = NULL;
struct task_struct *task;
@@ -49,6 +45,10 @@ static int fdinfo_open_helper(struct ino
*path = fd_file-f_path;
path_get(fd_file-f_path);
}
+   if (f_file) {
+   *f_file = fd_file;
+   get_file(fd_file);
+   }
ret = 0;
}
spin_unlock(files-file_lock);
@@ -58,43 +58,110 @@ static int fdinfo_open_helper(struct ino
return ret;
 }
 
+static void *seq_start(struct seq_file *m, loff_t *pos)
+{
+   struct proc_fdinfo_extra *extra = m-private;
+
+   extra-pos = *pos;
+
+   return *pos == 0 ? extra :
+   (extra-fdinfo_ops ?
+extra-fdinfo_ops-start(m, pos) : NULL);
+}
+
+static void seq_stop(struct seq_file *m, void *v)
+{
+   struct proc_fdinfo_extra *extra = m-private;
+
+   if (extra-fdinfo_ops  extra-pos  0)
+   extra-fdinfo_ops-stop(m, v);
+}
+
+static void *seq_next(struct seq_file *m, void *p, loff_t *pos)
+{
+   struct proc_fdinfo_extra *extra = m-private;
+   void *v = NULL;
+
+   if (extra-fdinfo_ops) {
+   int ret = 0;
+
+   if (*pos == 0) {
+   v = extra-fdinfo_ops-start(m, pos);
+   if (v) {
+   ret = extra-fdinfo_ops-show(m, v);
+   p = v;
+   } else
+   ret = -1;
+   }
+
+   if (!ret)
+   v = extra-fdinfo_ops-next(m, p, pos);
+   } else
+   ++*pos;
+
+   extra-pos = *pos;
+   return v;
+}
+
 static int seq_show(struct seq_file *m, void *v)
 {
-   struct proc_fdinfo *fdinfo = m-private;
+   struct proc_fdinfo_extra *extra = m-private;
+
+   if (extra-fdinfo_ops  extra-pos  0)
+   return extra-fdinfo_ops-show(m, v);
+
seq_printf(m, pos:\t%lli\nflags:\t0%o\n,
-  (long long)fdinfo-f_pos,
-  fdinfo-f_flags);
+  (long long)extra-f_file-f_pos,
+  extra-f_flags);
return 0;
 }
 
+static const struct seq_operations fdinfo_seq_ops = {
+   .start  = seq_start,
+   .next   = seq_next,
+   .stop   = seq_stop,
+   .show   = seq_show,
+};
+
 static int seq_fdinfo_open(struct inode *inode, struct file *file)
 {
-   struct proc_fdinfo *fdinfo = NULL;
-   int ret = -ENOENT;
+   struct proc_fdinfo_extra *extra;
+   struct seq_file *m;
+   int ret;
 
-   fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
-   if (!fdinfo)
+   extra = kzalloc(sizeof(*extra), GFP_KERNEL);
+   if (!extra)
return -ENOMEM;
 
-   ret = fdinfo_open_helper(inode, fdinfo-f_flags, NULL);
+   ret = seq_open(file, fdinfo_seq_ops);
if (!ret) {
-

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
 Al, does the patch below looks better? If so I'll fix up the rest.

  struct file_operations {
   struct module *owner;
 + struct seq_operations *fdinfo_ops;

IDGI.  Why on the earth do you need the whole iterator?  All it takes
is show_fdinfo(struct seq_file *m, struct file *f); have -show() in
your iterator (one going through the files) call that sucker for the
file we are trying to show.

I think you are severely overdesigning that thing...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Tue, Aug 14, 2012 at 10:27:21PM +0100, Al Viro wrote:
 On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
  Al, does the patch below looks better? If so I'll fix up the rest.
 
   struct file_operations {
  struct module *owner;
  +   struct seq_operations *fdinfo_ops;
 
 IDGI.  Why on the earth do you need the whole iterator?  All it takes
 is show_fdinfo(struct seq_file *m, struct file *f); have -show() in
 your iterator (one going through the files) call that sucker for the
 file we are trying to show.
 
 I think you are severely overdesigning that thing...

Hmm, in very first versions I've been using one -show method, but
then I thought that this is not very correlate with seq-files idea
where for each record show/next sequence is called. I'll update (this
for sure will make code simplier, and I'll have to check for seq-file
overflow after seq_printf call to not continue printing data for too
long if buffer already out of space).

Cyrill
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Cyrill Gorcunov 
On Wed, Aug 15, 2012 at 01:56:16AM +0400, Cyrill Gorcunov wrote:
struct file_operations {
 struct module *owner;
   + struct seq_operations *fdinfo_ops;
  
  IDGI.  Why on the earth do you need the whole iterator?  All it takes
  is show_fdinfo(struct seq_file *m, struct file *f); have -show() in
  your iterator (one going through the files) call that sucker for the
  file we are trying to show.
  
  I think you are severely overdesigning that thing...
 
 Hmm, in very first versions I've been using one -show method, but
 then I thought that this is not very correlate with seq-files idea
 where for each record show/next sequence is called. I'll update (this
 for sure will make code simplier, and I'll have to check for seq-file
 overflow after seq_printf call to not continue printing data for too
 long if buffer already out of space).

Al, I'll cook the whole series tomorrow and resend it for review,
also I guess the new show_fdinfo() member in file-operations should
be guarded with CONFIG_PROC_FS, right?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

2012-08-14 Thread Al Viro 
On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
  Hmm, in very first versions I've been using one -show method, but
  then I thought that this is not very correlate with seq-files idea
  where for each record show/next sequence is called. I'll update (this
  for sure will make code simplier, and I'll have to check for seq-file
  overflow after seq_printf call to not continue printing data for too
  long if buffer already out of space).
 
 Al, I'll cook the whole series tomorrow and resend it for review,
 also I guess the new show_fdinfo() member in file-operations should
 be guarded with CONFIG_PROC_FS, right?

I seriously doubt that it's worth bothering.  If somebody cares, they
can add making it conditional later.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/