driver core: Add the ability to unbind drivers to devices from userspace

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

Quoted 8 lines · Dmitry Torokhov

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

That will work around it but I think we should fix it. Changing to strncmp() fixes most cases.

Quoted 13 lines · Jon Smirl

Quoted 8 lines · Dmitry Torokhov

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

That will work around it but I think we should fix it. Changing to strncmp() fixes most cases.

Review

Quoted 3 lines

(quoted diff) +1 -1

-       if (strcmp(name, dev->bus_id) == 0)
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)

This will produce "interesting results" if you have both "blah-1" and "blah-10" devices on the bus.

Quoted 14 lines · Dmitry Torokhov

Quoted 13 lines · Jon Smirl

Quoted 8 lines · Dmitry Torokhov

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

That will work around it but I think we should fix it. Changing to strncmp() fixes most cases.

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

-       if (strcmp(name, dev->bus_id) == 0)
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)

This will produce "interesting results" if you have both "blah-1" and "blah-10" devices on the bus.

Then the better solution is to fix the generic attribute set code to strip leading and trailing white space.

Quoted 15 lines · Jon Smirl

Quoted 14 lines · Dmitry Torokhov

Quoted 13 lines · Jon Smirl

Quoted 8 lines · Dmitry Torokhov

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

That will work around it but I think we should fix it. Changing to strncmp() fixes most cases.

Quoted 6 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

-       if (strcmp(name, dev->bus_id) == 0)
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)

This will produce "interesting results" if you have both "blah-1" and "blah-10" devices on the bus.

Yes, not a good thing for USB devices specifically.

Quoted 2 lines

Then the better solution is to fix the generic attribute set code to strip leading and trailing white space.

No, that might break other things as we have not been doing this from day one. I'd rather just change these two places, if it's that big of a deal. It was documented (in a lwn.net article) and the changelog entry, that you should use "echo -n".

Quoted 25 lines · Greg KH

Quoted 15 lines · Jon Smirl

Quoted 14 lines · Dmitry Torokhov

Quoted 13 lines · Jon Smirl

Quoted 8 lines · Dmitry Torokhov

Quoted 5 lines · Jon Smirl

I just pulled from GIT to test bind/unbind. I couldn't get it to work; it isn't taking into account the CR on the end of the input value of the sysfs attribute. This patch will fix it but I'm sure there is a cleaner solution.

"echo -n" should take care of this problem I think.

That will work around it but I think we should fix it. Changing to strncmp() fixes most cases.

Quoted 6 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -1

-       if (strcmp(name, dev->bus_id) == 0)
+       if (strncmp(name, dev->bus_id, strlen(dev->bus_id)) == 0)

This will produce "interesting results" if you have both "blah-1" and "blah-10" devices on the bus.

Yes, not a good thing for USB devices specifically.

Quoted 7 lines

Quoted 2 lines

Then the better solution is to fix the generic attribute set code to strip leading and trailing white space.

No, that might break other things as we have not been doing this from day one. I'd rather just change these two places, if it's that big of a deal. It was documented (in a lwn.net article) and the changelog entry, that you should use "echo -n".

I didn't realize that echo was adding the CR, I thought that it always appeared on the end of a sysfs attribute set. So now I have to go add white space stripping to a dozen fbdev/drm sysfs attribute implementations. Given that the param is const I may have to allocate new buffers and copy. I also wonder how many other people have made the same mistake.

Are you sure it would break other things? These are supposed to be text attributes, not binary ones.

Quoted 6 lines · Jon Smirl

I didn't realize that echo was adding the CR, I thought that it always appeared on the end of a sysfs attribute set. So now I have to go add white space stripping to a dozen fbdev/drm sysfs attribute implementations. Given that the param is const I may have to allocate new buffers and copy. I also wonder how many other people have made the same mistake.

Nah, just zero out that \n character :)

Quoted 3 lines

Are you sure it would break other things? These are supposed to be text attributes, not binary ones.

I agree, I don't know what would break. Care to make a patch so we could find out?

Quoted 8 lines · Greg KH

Quoted 6 lines · Jon Smirl

I didn't realize that echo was adding the CR, I thought that it always appeared on the end of a sysfs attribute set. So now I have to go add white space stripping to a dozen fbdev/drm sysfs attribute implementations. Given that the param is const I may have to allocate new buffers and copy. I also wonder how many other people have made the same mistake.

Quoted 1 line

Nah, just zero out that \n character :)

The input buffer is "const char * buf". I will have to override the const to zero it out.

Quoted 5 lines

Quoted 2 lines

Are you sure it would break other things? These are supposed to be text attributes, not binary ones.

I agree, I don't know what would break. Care to make a patch so we could find out?

I'll put one together to trim leading/trailing white space from the buffer before it is passed into the attribute functions. Now that I think about this I believe the attributes should have always had the leading/trailing white space removed. If we don't do it in the sysfs code then every driver has to do it.

Quoted 5 lines

I'll put one together to trim leading/trailing white space from the buffer before it is passed into the attribute functions. Now that I think about this I believe the attributes should have always had the leading/trailing white space removed. If we don't do it in the sysfs code then every driver has to do it.

Ok, sounds good.

Quoted 7 lines · Greg KH

Quoted 5 lines

I'll put one together to trim leading/trailing white space from the buffer before it is passed into the attribute functions. Now that I think about this I believe the attributes should have always had the leading/trailing white space removed. If we don't do it in the sysfs code then every driver has to do it.

Ok, sounds good.

How does this look? This is a count based interface but a lot of attributes don't work unless I add the terminating zero. This interface should be documented: count or zero terminated, white space stripped or not, etc. Are these strings ASCII, UTF8, Unicode?

fs/sysfs/file.c +23 -1

@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
  #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 

@@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
        struct attribute * attr = to_attr(dentry);
        struct kobject * kobj = to_kobj(dentry->d_parent);
        struct sysfs_ops * ops = buffer->ops;
+       char *x, *y, *z;
+
+       /* locate leading white space */
+       x = buffer->page;
+       while( isspace(*x) && (x - buffer->page < count))
+               x++;
+
+       /* locate trailng white space */
+       z = y = x;
+       while (y - buffer->page < count) {
+               y++;
+               z = y;
+               while( isspace(*y) && (y - buffer->page < count)) {
+                       y++;
+               }
+       }
+       count = z - x;
+
+       /* strip the white space */
+       if (buffer->page != x)
+               memmove(buffer->page, x, count);
+       buffer->page[count] = '\0';
 
        return ops->store(kobj,attr,buffer->page,count);
 }
 
 
--

Jon Smirl
[email protected]

Any comments on this? I'll fix up the whitespace issues if everyone agrees that the code works.

New patch with fixed whitespace.

Quoted 5 lines

fs/sysfs/file.c +5 -0

+	/* locate trailng white space */
+	z = y = x;
+	while (y - buffer->page < count) {
+		y++;
+		z = y;

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 227

Quoted 5 lines

fs/sysfs/file.c +5 -0

+		while (isspace(*y) && (y - buffer->page < count)) {
+			y++;
+		}
+	}
+	count = z - x;

Hm, I _think_ this works, but I need someone else to verify this... Anyone else?

Quoted 11 lines · Greg KH

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 227

Quoted 10 lines

fs/sysfs/file.c +10 -0

+   /* locate trailng white space */
+   z = y = x;
+   while (y - buffer->page < count) {
+           y++;
+           z = y;
+           while (isspace(*y) && (y - buffer->page < count)) {
+                   y++;
+           }
+   }
+   count = z - x;

Quoted 2 lines

Hm, I _think_ this works, but I need someone else to verify this... Anyone else?

It looks sane-ish to me, but also more complicated than need be. Why can't you just do something like:

        while (count > 0 && isspace(x[count - 1]))
                count--;

-Mitch

Quoted 21 lines

Quoted 11 lines · Greg KH

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 227

Quoted 10 lines

fs/sysfs/file.c +10 -0

+   /* locate trailng white space */
+   z = y = x;
+   while (y - buffer->page < count) {
+           y++;
+           z = y;
+           while (isspace(*y) && (y - buffer->page < count)) {
+                   y++;
+           }
+   }
+   count = z - x;

Quoted 2 lines

Hm, I _think_ this works, but I need someone else to verify this... Anyone else?

It looks sane-ish to me, but also more complicated than need be. Why can't you just do something like:

        while (count > 0 && isspace(x[count - 1]))
                count--;

Quoted 2 lines

-Mitch

Do we need to deal with UTF8 here? I did the forward loop because you can't parse UTF8 backwards. If UTF8 is possible I need to change the pointer inc function.

Quoted 6 lines · Mitchell Blank Jr

[...]
It looks sane-ish to me, but also more complicated than need be. Why can't you just do something like:

       while (count > 0 && isspace(x[count - 1]))
               count--;

Do we need to deal with UTF8 here? I did the forward loop because you can't parse UTF8 backwards. If UTF8 is possible I need to change the pointer inc function.

Do we need to deal with UTF8 here? I did the forward loop because you can't parse UTF8 backwards. If UTF8 is possible I need to change the pointer inc function.

Quoted 21 lines · Mitchell Blank Jr

Quoted 11 lines · Greg KH

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 227

Quoted 10 lines

fs/sysfs/file.c +10 -0

+   /* locate trailng white space */
+   z = y = x;
+   while (y - buffer->page < count) {
+           y++;
+           z = y;
+           while (isspace(*y) && (y - buffer->page < count)) {
+                   y++;
+           }
+   }
+   count = z - x;

Quoted 2 lines

Hm, I _think_ this works, but I need someone else to verify this... Anyone else?

It looks sane-ish to me, but also more complicated than need be. Why can't you just do something like:

        while (count > 0 && isspace(x[count - 1]))
                count--;

Quoted 2 lines

-Mitch

Do we need to deal with UTF8 here? I did the forward loop because you can't parse UTF8 backwards. If UTF8 is possible I need to change the pointer inc function.

Even simpler version....

--
Jon Smirl
[email protected]

Remove leading and trailing whitespace when text sysfs attribute is set

Signed-off-by Jon Smirl <[email protected]>

fs/sysfs/file.c +17 -1

@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 

@@ -207,8 +208,23 @@ flush_write_buffer(struct dentry * dentr
        struct attribute * attr = to_attr(dentry);
        struct kobject * kobj = to_kobj(dentry->d_parent);
        struct sysfs_ops * ops = buffer->ops;
+       char *x;
 
-       return ops->store(kobj,attr,buffer->page,count);
+       /* locate trailing white space */
+       while ((count > 0) && isspace(buffer->page[count - 1]))
+               count--;
+
+       /* locate leading white space */
+       x = buffer->page;
+       if (count > 0) {
+               while (isspace(*x))
+                       x++;
+               count -= (x - buffer->page);
+       }
+       /* terminate the string */
+       x[count] = '\0';
+
+       return ops->store(kobj, attr, x, count);
 }
 

Greg, is this ok for your tree now or does it need more work?

Quoted 1 line · Jon Smirl

Greg, is this ok for your tree now or does it need more work?

It's in my queue, will add it to the tree next week. Sorry for the delay, was at OSCON this week...

Even simpler version....

--
Jon Smirl
[email protected]

Remove leading and trailing whitespace when text sysfs attribute is set

Signed-off-by Jon Smirl <[email protected]>

fs/sysfs/file.c +5 -1

@@ -6,6 +6,7 @@
 #include <linux/fsnotify.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
+#include <linux/ctype.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 

@@ -207,8 +208,23 @@ flush_write_buffer(struct dentry * dentr
        struct attribute * attr = to_attr(dentry);
        struct kobject * kobj = to_kobj(dentry->d_parent);
        struct sysfs_ops * ops = buffer->ops;
+       char *x;
 
-       return ops->store(kobj,attr,buffer->page,count);
+       /* locate trailing white space */
+       while ((count > 0) && isspace(buffer->page[count - 1]))
+               count--;

fs/sysfs/file.c +2 -0

+
+       return ops->store(kobj, attr, x, count);
 }
 

New, simplified version of the sysfs whitespace strip patch...

Am Donnerstag, 28. Juli 2005 21:57 schrieb Jon Smirl:

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Regards
Oliver

Quoted 5 lines

Am Donnerstag, 28. Juli 2005 21:57 schrieb Jon Smirl:

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Hi!

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Quoted 3 lines

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

Pavel
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms

Quoted 11 lines

Hi!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Quoted 2 lines

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:

Quoted 15 lines · Pavel Machek

Quoted 13 lines

Hi!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

Quoted 4 lines

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

The average user has no place poking sysfs. Root should know when to use -n, as should shell scripts.

Regards
Oliver

Quoted 21 lines

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:

Quoted 19 lines · Pavel Machek

Quoted 7 lines

Hi!

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Quoted 6 lines

Quoted 3 lines

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Quoted 2 lines

The average user has no place poking sysfs. Root should know when to use -n, as should shell scripts.

So the average user never needs to change their console mode? Check out /sys/class/graphics/fb/modes and mode.

Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:

Quoted 22 lines · Oliver Neukum

Quoted 21 lines

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:

Quoted 19 lines · Pavel Machek

Quoted 7 lines

Hi!

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Quoted 6 lines

Quoted 3 lines

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Quoted 5 lines

Quoted 2 lines

The average user has no place poking sysfs. Root should know when to use -n, as should shell scripts.

So the average user never needs to change their console mode? Check out /sys/class/graphics/fb/modes and mode.

That is what we have helper scripts for.

Quoted 29 lines

Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:

Quoted 27 lines · Oliver Neukum

Quoted 16 lines

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:

Quoted 15 lines · Pavel Machek

Quoted 13 lines

Hi!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

Quoted 7 lines

Quoted 4 lines

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

The average user has no place poking sysfs. Root should know when to use -n, as should shell scripts.

So the average user never needs to change their console mode? Check out /sys/class/graphics/fb/modes and mode.

Quoted 1 line

That is what we have helper scripts for.

If we are going back to needing helper scripts then I should just remove the entire sysfs graphics interface and switch back to using ioctls and a helper app. Of could no one can ever find the helper app or remember how it works. I thought one of the main reasons behind the sysfs interface was to eliminate these helper apps.

Without doing whitespace cleanup there is a 100% probability that this will generate bug reports. I know this for a fact because I am already getting them.

Am Freitag, 5. August 2005 20:47 schrieb Jon Smirl:

Quoted 30 lines · Oliver Neukum

Quoted 29 lines

Am Freitag, 5. August 2005 20:14 schrieb Jon Smirl:

Quoted 27 lines · Oliver Neukum

Quoted 16 lines

Am Freitag, 5. August 2005 15:32 schrieb Jon Smirl:

Quoted 15 lines · Pavel Machek

Quoted 13 lines

Hi!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

Quoted 7 lines

Quoted 4 lines

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

The average user has no place poking sysfs. Root should know when to use -n, as should shell scripts.

So the average user never needs to change their console mode? Check out /sys/class/graphics/fb/modes and mode.

Quoted 7 lines

Quoted 1 line

That is what we have helper scripts for.

If we are going back to needing helper scripts then I should just remove the entire sysfs graphics interface and switch back to using ioctls and a helper app. Of could no one can ever find the helper app or remember how it works. I thought one of the main reasons behind the sysfs interface was to eliminate these helper apps.

The point is that you _can_ do it with echo, not that it is _easy_. Nor is sysfs a solution in any case.

Quoted 3 lines

Without doing whitespace cleanup there is a 100% probability that this will generate bug reports. I know this for a fact because I am already getting them.

Stupid users are not a reason for kernel bloat.

Quoted 2 lines

Stupid users are not a reason for kernel bloat.

You have a very wrapped sense of kernel bloat. This is nine lines of code whose absence is guaranteed to generate a bunch of bug reports.

Not having it is also causing various implementers to implement attribute processing differently. Some are stripping white space in their implementations and some are not. If you want to attack kernel bloat there are much more productive areas.

If whitespace cleanup is rejected I believe we should eliminate text based sysfs attributes in general and make them all binary. I'll probably remove the fbdev sysfs interface because I don't want to deal with the bug reports reporting the same problem over and over.

Quoted 13 lines

Quoted 5 lines

If we are going back to needing helper scripts then I should just remove the entire sysfs graphics interface and switch back to using ioctls and a helper app. Of could no one can ever find the helper app or remember how it works. I thought one of the main reasons behind the sysfs interface was to eliminate these helper apps.

The point is that you _can_ do it with echo, not that it is _easy_. Nor is sysfs a solution in any case.

Quoted 3 lines

Without doing whitespace cleanup there is a 100% probability that this will generate bug reports. I know this for a fact because I am already getting them.

Quoted 1 line

Stupid users are not a reason for kernel bloat.

You have a very wrapped sense of kernel bloat. This is nine lines of code whose absence is guaranteed to generate a bunch of bug reports. Not having it is also causing various implementers to implement attribute processing differently. Some are stripping white space in their implementations and some are not. If you want to attack kernel

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Quoted 2 lines

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Hi!

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Could you tell me why you don't just fail the operation if malformed input is supplied?

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

Quoted 4 lines

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Kernel is not a place to be user friendly. Or do you propose stripping whitespace
for open(), too? File called "foo.txt " certainly *is* going to be confusing, but it should be allowed at kernel level.

Now... echo foo > /sys/var does not properly report errors. Thats bad, but it needs to be fixed in bash.

--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms

Quoted 7 lines

Quoted 4 lines

It is not a work around. These are text attributes meant for human use. Humans have a hard time cleaning up things they can't see. And the failure mode for this is awful, your attribute won't set but everything on the screen looks fine.

Kernel is not a place to be user friendly. Or do you propose stripping whitespace
for open(), too? File called "foo.txt " certainly *is* going to be confusing, but it should be allowed at kernel level.

open is not made for human use, it is used by programs and only indirectly by humans. sysfs variables are used by directly humans.

Quoted 2 lines

Now... echo foo > /sys/var does not properly report errors. Thats bad, but it needs to be fixed in bash.

It is going to take a lot more code to return an error that a parameter didn't match because of extra white space that it would take to simply remove the whitespace.

Hi!

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

New, simplified version of the sysfs whitespace strip patch...

Could you tell me why you don't just fail the operation if malformed input is supplied?

Quoted 3 lines

Leading/trailing white space should be allowed. For example echo appends '\n' unless you know to use -n. It is easier to fix the kernel than to teach everyone to use -n.

Please, NO! echo -n is the right thing to do, and users will eventually learn. We are not going to add such workarounds all over the kernel...

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 216

Quoted 10 lines · Jon Smirl

fs/sysfs/file.c +6 -0

@@ -207,6 +208,28 @@ flush_write_buffer(struct dentry * dentr
      struct attribute * attr = to_attr(dentry);
      struct kobject * kobj = to_kobj(dentry->d_parent);
      struct sysfs_ops * ops = buffer->ops;
+     char *x, *y, *z;
+
+     /* locate leading white space */
+     x = buffer->page;
+     while (isspace(*x) && (x - buffer->page < count))
+             x++;

Ok, I can follow this. For example
buffer->page = " foo "

then x = "foo " at the end of that .

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 227

Quoted 10 lines

fs/sysfs/file.c +10 -0

+     /* locate trailng white space */
+     z = y = x;
+     while (y - buffer->page < count) {
+             y++;
+             z = y;
+             while (isspace(*y) && (y - buffer->page < count)) {
+                     y++;
+             }
+     }
+     count = z - x;

Hm, I _think_ this works, but I need someone else to verify this... Anyone else?

fs/sysfs/file.c flush_write_buffer(struct dentry * dentr · line 232

Quoted 5 lines

fs/sysfs/file.c +5 -0

+
+     /* strip the white space */
+     if (buffer->page != x)
+             memmove(buffer->page, x, count);
+     buffer->page[count] = '\0';

Why move the buffer? Why not just pass in a pointer to the start of the "non-whitespace filled" buffer to the store() function?

thanks,

greg k-h