[PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[Denis Cheng]

this is especially useful after /sys/slab introduced, for example:

$ ls -l /sys/slab/mm_struct
lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> :0000448

instead of:

$ ls -l /sys/slab/mm_struct
lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> ../slab/:0000448

Signed-off-by: Denis Cheng <crquan@gmail.com>
---
 fs/sysfs/symlink.c |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 3eac20c..230a925 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -19,30 +19,41 @@
 
 #include "sysfs.h"
 
-static int object_depth(struct sysfs_dirent *sd)
+static struct sysfs_dirent *object_base(struct sysfs_dirent *parent,
+		struct sysfs_dirent *sd)
+{
+	for (; sd->s_parent; sd = sd->s_parent)
+		if (sd->s_parent == parent)
+			return parent;
+	return NULL;
+}
+
+static int object_depth(struct sysfs_dirent *base, struct sysfs_dirent *sd)
 {
 	int depth = 0;
 
-	for (; sd->s_parent; sd = sd->s_parent)
+	for (; sd->s_parent && sd != base; sd = sd->s_parent)
 		depth++;
 
 	return depth;
 }
 
-static int object_path_length(struct sysfs_dirent * sd)
+static int object_path_length(struct sysfs_dirent *base,
+		struct sysfs_dirent *sd)
 {
 	int length = 1;
 
-	for (; sd->s_parent; sd = sd->s_parent)
+	for (; sd->s_parent && sd != base; sd = sd->s_parent)
 		length += strlen(sd->s_name) + 1;
 
 	return length;
 }
 
-static void fill_object_path(struct sysfs_dirent *sd, char *buffer, int length)
+static void fill_object_path(struct sysfs_dirent *base,
+		struct sysfs_dirent *sd, char *buffer, int length)
 {
 	--length;
-	for (; sd->s_parent; sd = sd->s_parent) {
+	for (; sd->s_parent && sd != base; sd = sd->s_parent) {
 		int cur = strlen(sd->s_name);
 
 		/* back up enough to print this bus id with '/' */
@@ -129,18 +140,23 @@ static int sysfs_get_target_path(struct sysfs_dirent * parent_sd,
 {
 	char * s;
 	int depth, size;
+	struct sysfs_dirent *base;
 
-	depth = object_depth(parent_sd);
-	size = object_path_length(target_sd) + depth * 3 - 1;
+	base = object_base(parent_sd, target_sd);
+	depth = object_depth(base, parent_sd);
+	size = object_path_length(base, target_sd) + depth * 3 - 1;
 	if (size > PATH_MAX)
 		return -ENAMETOOLONG;
 
-	pr_debug("%s: depth = %d, size = %d\n", __FUNCTION__, depth, size);
+	pr_debug("%s: base = %s, depth = %d, size = %d\n",
+			__FUNCTION__,
+			base ? base->s_name : "/sys",
+			depth, size);
 
 	for (s = path; depth--; s += 3)
-		strcpy(s,"../");
+		strcpy(s, "../");
 
-	fill_object_path(target_sd, path, size);
+	fill_object_path(base, target_sd, path, size);
 	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
 
 	return 0;
-- 
1.5.3.4

Re: [PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[Greg KH]

On Wed, Oct 31, 2007 at 06:34:20PM +0800, Denis Cheng wrote:
> this is especially useful after /sys/slab introduced, for example:
> 
> $ ls -l /sys/slab/mm_struct
> lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> :0000448
> 
> instead of:
> 
> $ ls -l /sys/slab/mm_struct
> lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> ../slab/:0000448
> 
> Signed-off-by: Denis Cheng <crquan@gmail.com>

As pretty as this change is, it's not really necessary, right?

Is there any other place in /sys that would benefit from this?

thanks,

greg k-h

Re: [PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[rae l]

On 10/31/07, Greg KH <gregkh@suse.de> wrote:
> On Wed, Oct 31, 2007 at 06:34:20PM +0800, Denis Cheng wrote:
> > this is especially useful after /sys/slab introduced, for example:
> >
> > $ ls -l /sys/slab/mm_struct
> > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> :0000448
> >
> > instead of:
> >
> > $ ls -l /sys/slab/mm_struct
> > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> ../slab/:0000448
> >
> > Signed-off-by: Denis Cheng <crquan@gmail.com>
>
> As pretty as this change is, it's not really necessary, right?
I don't think so.

Suppose to create a symlink on the disk, say /usr/src/linux, that
points to /usr/src/linux-2.6.23, the best way is:
# cd /usr/src/
# ln -s linux-2.6.23 linux
# ls -l linux
# ls -l linux
lrwxrwxrwx 1 root root 14 2007-10-16 19:21 linux -> linux-2.6.23

other than:
# cd /usr/src/
# ln -s /usr/src/linux-2.6.23 linux
or
# ln -s ../../usr/src/linux-2.6.23 linux
# ls -l linux
lrwxrwxrwx 1 root root 14 2007-10-16 19:21 linux -> ../../usr/src/linux-2.6.23

Anyone know this, since sysfs is also a filesystem, it should conform
the perfect way.


For another point, consider the code in fs/sysfs/symlink.c:

static int sysfs_get_target_path(struct sysfs_dirent * parent_sd,
                                 struct sysfs_dirent * target_sd, char *path)
{
...
        size = object_path_length(target_sd) + depth * 3 - 1;
        if (size > PATH_MAX)
                return -ENAMETOOLONG;

Since having longer readlink result would consume more memory on the
output parameter path, that is error prone to return -ENAMETOOLONG; we
just need the shorter readlink result.

>
> Is there any other place in /sys that would benefit from this?
Yes. there are already some other symlinks those are also not crossing
top subdirectory of /sys, they would benefit from this patch:

I have found all of them by this little shell:

$ find /sys -type l -printf '%p -> %l -> ' -exec readlink -f '{}' \; | gawk '{
        split($1, a, "/");
        split($5, b, "/");
        if (a[3] == b[3])
                print;
}'

that will print many lines like:

...
/sys/block/hdd/subsystem -> ../../block -> /sys/block
/sys/module/snd_mixer_oss/holders/snd_pcm_oss ->
../../../module/snd_pcm_oss -> /sys/module/snd_pcm_oss
/sys/class/sound/audio/subsystem -> ../../../class/sound -> /sys/class/sound
/sys/class/pci_bus/0000:00/subsystem -> ../../../class/pci_bus ->
/sys/class/pci_bus
...

>
> thanks,
>
> greg k-h
>

-- 
Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
 - Ken Thompson.

Re: [PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[Kay Sievers]

On Oct 31, 2007 3:34 PM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Oct 31, 2007 at 06:34:20PM +0800, Denis Cheng wrote:
> > this is especially useful after /sys/slab introduced, for example:
> >
> > $ ls -l /sys/slab/mm_struct
> > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> :0000448
> >
> > instead of:
> >
> > $ ls -l /sys/slab/mm_struct
> > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> ../slab/:0000448
> >
> > Signed-off-by: Denis Cheng <crquan@gmail.com>
>
> As pretty as this change is, it's not really necessary, right?
>
> Is there any other place in /sys that would benefit from this?

Yeah:
  ep_81 -> ../../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-0:1.0/usb_endpoint/usbdev2.1_ep81

just becomes:
  ep_81 -> usb_endpoint/usbdev1.1_ep81

which is nice. The current logic always goes back down to the root of sysfs
and moves forward to the target, which in this case prepends the pointless:
  "../../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-0:1.0/" to
the target string.

Kay

Re: [PATCH] [sysfs]: make readlink result shorter when the symlink and its target shared some base sysfs subdirectory

[Kay Sievers]

On Thu, 2007-11-01 at 19:59 +0100, Kay Sievers wrote:
> On Oct 31, 2007 3:34 PM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Oct 31, 2007 at 06:34:20PM +0800, Denis Cheng wrote:
> > > this is especially useful after /sys/slab introduced, for example:
> > >
> > > $ ls -l /sys/slab/mm_struct
> > > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> :0000448
> > >
> > > instead of:
> > >
> > > $ ls -l /sys/slab/mm_struct
> > > lrwxrwxrwx 1 root root 0 2007-10-31 17:40 /sys/slab/mm_struct -> ../slab/:0000448
> > >
> > > Signed-off-by: Denis Cheng <crquan@gmail.com>
> >
> > As pretty as this change is, it's not really necessary, right?
> >
> > Is there any other place in /sys that would benefit from this?
> 
> Yeah:
>   ep_81 -> ../../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-0:1.0/usb_endpoint/usbdev2.1_ep81
> 
> just becomes:
>   ep_81 -> usb_endpoint/usbdev1.1_ep81
> 
> which is nice. The current logic always goes back down to the root of sysfs
> and moves forward to the target, which in this case prepends the pointless:
>   "../../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-0:1.0/" to
> the target string.

Here is what I get when reading all symlinks in the current kernel:
   $ for i in $(find /sys/ -type l); do readlink $i; done | wc
   1044    1047   37826

And with the optimized relative link targets:
   $ for i in $(find /sys/ -type l); do readlink $i; done | wc
   1044    1047   33584

It 's more than 4kb less of "../" + redundant directory names. :)

Here is a bit simpler patch, that also removes more code from the kernel
than it adds, and does the optimized link targets. Maybe it adds more
bugs than it removes too. :)

Thanks,
Kay



From: Kay Sievers <kay.sievers@vrfy.org>
Subject: sysfs: create optimal relative symlink targets

Instead of walking from the source down to the root of sysfs, and back
to the target, we stop at the first directory the source and the target
share.

This link:
  /devices/pci0000:00/0000:00:1d.7/usb1/1-0:1.0/ep_81

pointed to:
  ../../../../../devices/pci0000:00/0000:00:1d.0/usb2/2-0:1.0/usb_endpoint/usbdev2.1_ep81

now it just points to:
  usb_endpoint/usbdev1.1_ep81

Thanks to Denis Cheng for bringing this up, and sending the initial patch.

CC: Denis Cheng <crquan@gmail.com>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---

 symlink.c |   88 +++++++++++++++++++++++++++++---------------------------------
 1 file changed, 42 insertions(+), 46 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 3eac20c..5f66c44 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -19,39 +19,6 @@
 
 #include "sysfs.h"
 
-static int object_depth(struct sysfs_dirent *sd)
-{
-	int depth = 0;
-
-	for (; sd->s_parent; sd = sd->s_parent)
-		depth++;
-
-	return depth;
-}
-
-static int object_path_length(struct sysfs_dirent * sd)
-{
-	int length = 1;
-
-	for (; sd->s_parent; sd = sd->s_parent)
-		length += strlen(sd->s_name) + 1;
-
-	return length;
-}
-
-static void fill_object_path(struct sysfs_dirent *sd, char *buffer, int length)
-{
-	--length;
-	for (; sd->s_parent; sd = sd->s_parent) {
-		int cur = strlen(sd->s_name);
-
-		/* back up enough to print this bus id with '/' */
-		length -= cur;
-		strncpy(buffer + length, sd->s_name, cur);
-		*(buffer + --length) = '/';
-	}
-}
-
 /**
  *	sysfs_create_link - create symlink between two objects.
  *	@kobj:	object whose directory we're creating the link in.
@@ -112,7 +79,6 @@ int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char
 	return error;
 }
 
-
 /**
  *	sysfs_remove_link - remove symlink in object's directory.
  *	@kobj:	object we're acting for.
@@ -124,24 +90,54 @@ void sysfs_remove_link(struct kobject * kobj, const char * name)
 	sysfs_hash_and_remove(kobj->sd, name);
 }
 
-static int sysfs_get_target_path(struct sysfs_dirent * parent_sd,
-				 struct sysfs_dirent * target_sd, char *path)
+static int sysfs_get_target_path(struct sysfs_dirent *parent_sd,
+				 struct sysfs_dirent *target_sd, char *path)
 {
-	char * s;
-	int depth, size;
+	struct sysfs_dirent *base, *sd;
+	char *s = path;
+	int len = 0;
+
+	/* go up to the root, stop at the base */
+	base = parent_sd;
+	while (base->s_parent) {
+		sd = target_sd->s_parent;
+		while (sd->s_parent && base != sd)
+			sd = sd->s_parent;
+
+		if (base == sd)
+			break;
+
+		strcpy(s, "../");
+		s += 3;
+		base = base->s_parent;
+	}
+
+	/* determine end of target string for reverse fillup */
+	sd = target_sd;
+	while (sd->s_parent && sd != base) {
+		len += strlen(sd->s_name) + 1;
+		sd = sd->s_parent;
+	}
 
-	depth = object_depth(parent_sd);
-	size = object_path_length(target_sd) + depth * 3 - 1;
-	if (size > PATH_MAX)
+	/* check limits */
+	if (len < 2)
+		return -EINVAL;
+	len--;
+	if ((s - path) + len > PATH_MAX)
 		return -ENAMETOOLONG;
 
-	pr_debug("%s: depth = %d, size = %d\n", __FUNCTION__, depth, size);
+	/* reverse fillup of target string from target to base */
+	sd = target_sd;
+	while (sd->s_parent && sd != base) {
+		int slen = strlen(sd->s_name);
 
-	for (s = path; depth--; s += 3)
-		strcpy(s,"../");
+		len -= slen;
+		strncpy(s + len, sd->s_name, slen);
+		if (len)
+			s[--len] = '/';
 
-	fill_object_path(target_sd, path, size);
-	pr_debug("%s: path = '%s'\n", __FUNCTION__, path);
+		sd = sd->s_parent;
+	}
 
 	return 0;
 }