[PATCH debugfs: implement symbolic links

[PATCH debugfs: implement symbolic links

[Peter Oberparleiter]

From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>

debugfs: implement symbolic links

Implement a new function debugfs_create_symlink() which can be used
to create symbolic links in debugfs. This function can be useful
for people moving functionality from /proc to debugfs (e.g. the
gcov-kernel patch).

Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
---

 fs/debugfs/file.c       |   12 ++++++
 fs/debugfs/inode.c      |   76 ++++++++++++++++++++++++++++++++++++--
 include/linux/debugfs.h |   10 +++++
 3 files changed, 94 insertions(+), 4 deletions(-)

diff -uprN linux-2.6.20/fs/debugfs/file.c linux-2.6.20-patched/fs/debugfs/file.c
--- linux-2.6.20/fs/debugfs/file.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-patched/fs/debugfs/file.c	2007-02-13 10:55:38.000000000 +0100
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/pagemap.h>
+#include <linux/namei.h>
 #include <linux/debugfs.h>
 
 static ssize_t default_read_file(struct file *file, char __user *buf,
@@ -44,6 +45,17 @@ const struct file_operations debugfs_fil
 	.open =		default_open,
 };
 
+static void *debugfs_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	nd_set_link(nd, dentry->d_inode->i_private);
+	return NULL;
+}
+
+const struct inode_operations debugfs_link_operations = {
+	.readlink       = generic_readlink,
+	.follow_link    = debugfs_follow_link,
+};
+
 static void debugfs_u8_set(void *data, u64 val)
 {
 	*(u8 *)data = val;
diff -uprN linux-2.6.20/fs/debugfs/inode.c linux-2.6.20-patched/fs/debugfs/inode.c
--- linux-2.6.20/fs/debugfs/inode.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-patched/fs/debugfs/inode.c	2007-02-13 10:58:00.000000000 +0100
@@ -25,11 +25,13 @@
 #include <linux/namei.h>
 #include <linux/debugfs.h>
 #include <linux/fsnotify.h>
+#include <linux/string.h>
 
 #define DEBUGFS_MAGIC	0x64626720
 
 /* declared over in file.c */
 extern struct file_operations debugfs_file_operations;
+extern struct inode_operations debugfs_link_operations;
 
 static struct vfsmount *debugfs_mount;
 static int debugfs_mount_count;
@@ -51,6 +53,9 @@ static struct inode *debugfs_get_inode(s
 		case S_IFREG:
 			inode->i_fop = &debugfs_file_operations;
 			break;
+		case S_IFLNK:
+			inode->i_op = &debugfs_link_operations;
+			break;
 		case S_IFDIR:
 			inode->i_op = &simple_dir_inode_operations;
 			inode->i_fop = &simple_dir_operations;
@@ -96,6 +101,12 @@ static int debugfs_mkdir(struct inode *d
 	return res;
 }
 
+static int debugfs_link(struct inode *dir, struct dentry *dentry, int mode)
+{
+	mode = (mode & S_IALLUGO) | S_IFLNK;
+	return debugfs_mknod(dir, dentry, mode, 0);
+}
+
 static int debugfs_create(struct inode *dir, struct dentry *dentry, int mode)
 {
 	int res;
@@ -158,10 +169,17 @@ static int debugfs_create_by_name(const 
 	mutex_lock(&parent->d_inode->i_mutex);
 	*dentry = lookup_one_len(name, parent, strlen(name));
 	if (!IS_ERR(*dentry)) {
-		if ((mode & S_IFMT) == S_IFDIR)
+		switch (mode & S_IFMT) {
+		case S_IFDIR:
 			error = debugfs_mkdir(parent->d_inode, *dentry, mode);
-		else 
+			break;
+		case S_IFLNK:
+			error = debugfs_link(parent->d_inode, *dentry, mode);
+			break;
+		default:
 			error = debugfs_create(parent->d_inode, *dentry, mode);
+			break;
+		}
 		dput(*dentry);
 	} else
 		error = PTR_ERR(*dentry);
@@ -259,6 +277,49 @@ struct dentry *debugfs_create_dir(const 
 EXPORT_SYMBOL_GPL(debugfs_create_dir);
 
 /**
+ * debugfs_create_symlink- create a symbolic link in the debugfs filesystem
+ * @name: a pointer to a string containing the name of the symbolic link to
+ *        create.
+ * @parent: a pointer to the parent dentry for this symbolic link.  This
+ *          should be a directory dentry if set.  If this paramater is NULL,
+ *          then the symbolic link will be created in the root of the debugfs
+ *          filesystem.
+ * @target: a pointer to a string containing the path to the target of the
+ *          symbolic link.
+ *
+ * This function creates a symbolic link with the given name in debugfs that
+ * links to the given target path.
+ *
+ * This function will return a pointer to a dentry if it succeeds.  This
+ * pointer must be passed to the debugfs_remove() function when the symbolic
+ * link is to be removed (no automatic cleanup happens if your module is
+ * unloaded, you are responsible here.)  If an error occurs, %NULL will be
+ * returned.
+ *
+ * If debugfs is not enabled in the kernel, the value -%ENODEV will be
+ * returned.  It is not wise to check for this value, but rather, check for
+ * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
+ * code.
+ */
+struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
+				      const char *target)
+{
+	struct dentry *result;
+	char *link;
+
+	link = kstrdup(target, GFP_KERNEL);
+	if (!link)
+		return NULL;
+
+	result = debugfs_create_file(name, S_IFLNK | S_IRWXUGO, parent, link,
+				     NULL);
+	if (!result)
+		kfree(link);
+	return result;
+}
+EXPORT_SYMBOL_GPL(debugfs_create_symlink);
+
+/**
  * debugfs_remove - removes a file or directory from the debugfs filesystem
  * @dentry: a pointer to a the dentry of the file or directory to be
  *          removed.
@@ -287,15 +348,22 @@ void debugfs_remove(struct dentry *dentr
 	if (debugfs_positive(dentry)) {
 		if (dentry->d_inode) {
 			dget(dentry);
-			if (S_ISDIR(dentry->d_inode->i_mode)) {
+			switch (dentry->d_inode->i_mode & S_IFMT) {
+			case S_IFDIR:
 				ret = simple_rmdir(parent->d_inode, dentry);
 				if (ret)
 					printk(KERN_ERR
 						"DebugFS rmdir on %s failed : "
 						"directory not empty.\n",
 						dentry->d_name.name);
-			} else
+				break;
+			case S_IFLNK:
+				kfree(dentry->d_inode->i_private);
+				/* fall through */
+			default:
 				simple_unlink(parent->d_inode, dentry);
+				break;
+			}
 			if (!ret)
 				d_delete(dentry);
 			dput(dentry);
diff -uprN linux-2.6.20/include/linux/debugfs.h linux-2.6.20-patched/include/linux/debugfs.h
--- linux-2.6.20/include/linux/debugfs.h	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-patched/include/linux/debugfs.h	2007-02-13 10:57:10.000000000 +0100
@@ -33,6 +33,9 @@ struct dentry *debugfs_create_file(const
 
 struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);
 
+struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
+				      const char *dest);
+
 void debugfs_remove(struct dentry *dentry);
 
 struct dentry *debugfs_create_u8(const char *name, mode_t mode,
@@ -70,6 +73,13 @@ static inline struct dentry *debugfs_cre
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct dentry *debugfs_create_symlink(const char *name,
+						    struct dentry *parent,
+						    const char *dest)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline void debugfs_remove(struct dentry *dentry)
 { }
 

Re: [PATCH debugfs: implement symbolic links

[Cornelia Huck]

On Tue, 13 Feb 2007 12:13:54 +0100,
Peter Oberparleiter <peter.oberparleiter@de.ibm.com> wrote:

Not especially related to this patch (which just does the same as the
other debugfs functions), but:

> + * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> + * returned.  It is not wise to check for this value, but rather, check for
> + * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
> + * code.

does not look like good advice for return code handling. Return code
seems to be:

- ERR_PTR(-ENODEV) if debugfs is disabled
- NULL if debugfs is enabled and something went wrong
- !NULL and !IS_ERR if debugfs is enabled and all went fine

That makes it easy to get return code checking wrong (especially
considering the comment above), and a number of callers do get it wrong.

How about changing the return code behaviour of the debugfs code, either

1. return NULL if debugfs is disabled or something went wrong, !NULL
   else or
2. return ERR_PTR(-ENODEV) if debugfs is disabled, ERR_PTR(-ESOMEERROR)
   if something went wrong or a proper dentry if everything went fine?

At the very least we should change the misleading comment.

-- 
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: cornelia.huck@de.ibm.com

Re: [PATCH debugfs: implement symbolic links

[Greg KH]

On Tue, Feb 13, 2007 at 12:13:54PM +0100, Peter Oberparleiter wrote:
> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> 
> debugfs: implement symbolic links
> 
> Implement a new function debugfs_create_symlink() which can be used
> to create symbolic links in debugfs. This function can be useful
> for people moving functionality from /proc to debugfs (e.g. the
> gcov-kernel patch).

You aren't really creating debugfs symlinks from /sys/kernel/debug/ to
the /proc directory are you?  That's pretty insane...

Otherwise I have no objection to this, I'll add it to my queue...

thanks,

greg k-h

Re: [PATCH debugfs: implement symbolic links

[Greg KH]

On Tue, Feb 13, 2007 at 04:45:51PM +0100, Cornelia Huck wrote:
> On Tue, 13 Feb 2007 12:13:54 +0100,
> Peter Oberparleiter <peter.oberparleiter@de.ibm.com> wrote:
> 
> Not especially related to this patch (which just does the same as the
> other debugfs functions), but:
> 
> > + * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> > + * returned.  It is not wise to check for this value, but rather, check for
> > + * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
> > + * code.
> 
> does not look like good advice for return code handling. Return code
> seems to be:
> 
> - ERR_PTR(-ENODEV) if debugfs is disabled
> - NULL if debugfs is enabled and something went wrong
> - !NULL and !IS_ERR if debugfs is enabled and all went fine
> 
> That makes it easy to get return code checking wrong (especially
> considering the comment above), and a number of callers do get it wrong.

They do?

The goal here is not to force the caller to care if debugfs is enabled
or not.

> How about changing the return code behaviour of the debugfs code, either
> 
> 1. return NULL if debugfs is disabled or something went wrong, !NULL
>    else or
> 2. return ERR_PTR(-ENODEV) if debugfs is disabled, ERR_PTR(-ESOMEERROR)
>    if something went wrong or a proper dentry if everything went fine?

Your proposal changes the logic, if NULL is returned, callers will not
know if this is just because debugfs is disabled (and they can continue
on just fine), or if a real error happened.

> At the very least we should change the misleading comment.

agreed, patches always welcome :)

thanks,

greg k-h

Re: [PATCH debugfs: implement symbolic links

[Cornelia Huck]

On Tue, 13 Feb 2007 17:31:42 -0800,
Greg KH <gregkh@suse.de> wrote:

> > That makes it easy to get return code checking wrong (especially
> > considering the comment above), and a number of callers do get it wrong.
> 
> They do?

For example, fs/ocfs2/super.c only checks for NULL (while neither
selecting nor depending on debugfs), or drivers/block/pktcdvd.c will
only check for IS_ERR(). (Many callers don't seem to care about return
codes at all.)

> The goal here is not to force the caller to care if debugfs is enabled
> or not.

And that's definetly a good thing. (Looking again, not checking for
IS_ERR() isn't as bad as I thought, as the code will continue to do
nothing if called again for a non-existing dentry.)

> > At the very least we should change the misleading comment.
> 
> agreed, patches always welcome :)

OK, just changing the comment looks like the most sensible thing to do.
I'll roll up a patch.

[Patch] debugfs: Remove misleading comments.

[Cornelia Huck]

[This goes on top of Peter's symlink patch.]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

Just mention which error will be returned if debugfs is disabled. Callers
should be able to figure out themselves what they need to check.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

---
 fs/debugfs/inode.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

--- linux-2.6.orig/fs/debugfs/inode.c
+++ linux-2.6/fs/debugfs/inode.c
@@ -212,9 +212,7 @@ static int debugfs_create_by_name(const 
  * you are responsible here.)  If an error occurs, %NULL will be returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned.  It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * returned.
  */
 struct dentry *debugfs_create_file(const char *name, mode_t mode,
 				   struct dentry *parent, void *data,
@@ -264,9 +262,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
  * you are responsible here.)  If an error occurs, %NULL will be returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned.  It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * returned.
  */
 struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 {
@@ -297,9 +293,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir);
  * returned.
  *
  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
- * returned.  It is not wise to check for this value, but rather, check for
- * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
- * code.
+ * returned.
  */
 struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *target)

Re: [PATCH debugfs: implement symbolic links

[Peter 1 Oberparleiter]

Greg KH <gregkh@suse.de> wrote on 14.02.2007 02:27:32:
> On Tue, Feb 13, 2007 at 12:13:54PM +0100, Peter Oberparleiter wrote:
> > This function can be useful
> > for people moving functionality from /proc to debugfs (e.g. the
> > gcov-kernel patch).
> 
> You aren't really creating debugfs symlinks from /sys/kernel/debug/ to
> the /proc directory are you?  That's pretty insane...

Interesting idea :) But I was thinking more along the lines of replacing
existing /proc files and symlinks with files and links in debugfs.

> Otherwise I have no objection to this, I'll add it to my queue...

Great, thanks!


Regards,
  Peter Oberparleiter

Re: [PATCH debugfs: implement symbolic links

[Greg KH]

On Wed, Feb 14, 2007 at 09:03:24AM +0100, Peter 1 Oberparleiter wrote:
> Greg KH <gregkh@suse.de> wrote on 14.02.2007 02:27:32:
> > On Tue, Feb 13, 2007 at 12:13:54PM +0100, Peter Oberparleiter wrote:
> > > This function can be useful
> > > for people moving functionality from /proc to debugfs (e.g. the
> > > gcov-kernel patch).
> > 
> > You aren't really creating debugfs symlinks from /sys/kernel/debug/ to
> > the /proc directory are you?  That's pretty insane...
> 
> Interesting idea :) But I was thinking more along the lines of replacing
> existing /proc files and symlinks with files and links in debugfs.

Ah, ok, that sounds much saner :)

thanks,

greg k-h