error handling in sysfs, fill_read_buffer()

error handling in sysfs, fill_read_buffer()

[Oliver Neukum]

Hi,

if a driver returns an error in fill_read_buffer(), the buffer will be
marked as filled. Subsequent reads will return eof. But there is
no data because of an error, not because it has been read.
Not marking the buffer filled is the obvious fix.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oliver@neukum.name>
--

--- a/fs/sysfs/file.c	2006-12-24 05:00:32.000000000 +0100
+++ b/fs/sysfs/file.c	2007-01-01 15:03:14.000000000 +0100
@@ -70,7 +70,8 @@
  *	Allocate @buffer->page, if it hasn't been already, then call the
  *	kobject's show() method to fill the buffer with this attribute's 
  *	data. 
- *	This is called only once, on the file's first read. 
+ *	This is called only once, on the file's first read unless an error
+ *	is returned.
  */
 static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer)
 {
@@ -88,12 +89,13 @@
 
 	buffer->event = atomic_read(&sd->s_event);
 	count = ops->show(kobj,attr,buffer->page);
-	buffer->needs_read_fill = 0;
 	BUG_ON(count > (ssize_t)PAGE_SIZE);
-	if (count >= 0)
+	if (count >= 0) {
+		buffer->needs_read_fill = 0;
 		buffer->count = count;
-	else
+	} else {
 		ret = count;
+	}
 	return ret;
 }
 

Re: error handling in sysfs, fill_read_buffer()

[Alan Stern]

On Tue, 2 Jan 2007, Oliver Neukum wrote:

> Hi,
> 
> if a driver returns an error in fill_read_buffer(), the buffer will be
> marked as filled. Subsequent reads will return eof. But there is
> no data because of an error, not because it has been read.
> Not marking the buffer filled is the obvious fix.
> 
> 	Regards
> 		Oliver
> 
> Signed-off-by: Oliver Neukum <oliver@neukum.name>
> --
> 
> --- a/fs/sysfs/file.c	2006-12-24 05:00:32.000000000 +0100
> +++ b/fs/sysfs/file.c	2007-01-01 15:03:14.000000000 +0100
> @@ -70,7 +70,8 @@
>   *	Allocate @buffer->page, if it hasn't been already, then call the
>   *	kobject's show() method to fill the buffer with this attribute's 
>   *	data. 
> - *	This is called only once, on the file's first read. 
> + *	This is called only once, on the file's first read unless an error
> + *	is returned.
>   */

I don't think this matches what people expect of sysfs.  If a show method 
fails then the assumption is that the file cannot be read at all, so 
there's no point in trying to call the method again.

Alan Stern

Re: error handling in sysfs, fill_read_buffer()

[Oliver Neukum]

Am Dienstag, 2. Januar 2007 16:26 schrieb Alan Stern:
> On Tue, 2 Jan 2007, Oliver Neukum wrote:
> 
> > Hi,
> > 
> > if a driver returns an error in fill_read_buffer(), the buffer will be
> > marked as filled. Subsequent reads will return eof. But there is
> > no data because of an error, not because it has been read.
> > Not marking the buffer filled is the obvious fix.
> > 
> > 	Regards
> > 		Oliver
> > 
> > Signed-off-by: Oliver Neukum <oliver@neukum.name>
> > --
> > 
> > --- a/fs/sysfs/file.c	2006-12-24 05:00:32.000000000 +0100
> > +++ b/fs/sysfs/file.c	2007-01-01 15:03:14.000000000 +0100
> > @@ -70,7 +70,8 @@
> >   *	Allocate @buffer->page, if it hasn't been already, then call the
> >   *	kobject's show() method to fill the buffer with this attribute's 
> >   *	data. 
> > - *	This is called only once, on the file's first read. 
> > + *	This is called only once, on the file's first read unless an error
> > + *	is returned.
> >   */
> 
> I don't think this matches what people expect of sysfs.  If a show method 
> fails then the assumption is that the file cannot be read at all, so 
> there's no point in trying to call the method again.

This would make handling ENOMEM very hard.

	Regards
		Oliver

Re: error handling in sysfs, fill_read_buffer()

[Alan Stern]

On Tue, 2 Jan 2007, Oliver Neukum wrote:

> Am Dienstag, 2. Januar 2007 16:26 schrieb Alan Stern:
> > On Tue, 2 Jan 2007, Oliver Neukum wrote:
> > 
> > > Hi,
> > > 
> > > if a driver returns an error in fill_read_buffer(), the buffer will be
> > > marked as filled. Subsequent reads will return eof. But there is
> > > no data because of an error, not because it has been read.
> > > Not marking the buffer filled is the obvious fix.
> > > 
> > > 	Regards
> > > 		Oliver
> > > 
> > > Signed-off-by: Oliver Neukum <oliver@neukum.name>
> > > --
> > > 
> > > --- a/fs/sysfs/file.c	2006-12-24 05:00:32.000000000 +0100
> > > +++ b/fs/sysfs/file.c	2007-01-01 15:03:14.000000000 +0100
> > > @@ -70,7 +70,8 @@
> > >   *	Allocate @buffer->page, if it hasn't been already, then call the
> > >   *	kobject's show() method to fill the buffer with this attribute's 
> > >   *	data. 
> > > - *	This is called only once, on the file's first read. 
> > > + *	This is called only once, on the file's first read unless an error
> > > + *	is returned.
> > >   */
> > 
> > I don't think this matches what people expect of sysfs.  If a show method 
> > fails then the assumption is that the file cannot be read at all, so 
> > there's no point in trying to call the method again.
> 
> This would make handling ENOMEM very hard.

No harder than handling any other error: Close the sysfs file, then open 
it and try to read it again.

On the other hand, I agree there's nothing substantively wrong with the 
patch.

Alan Stern

Re: error handling in sysfs, fill_read_buffer()

[Oliver Neukum]

Am Dienstag, 2. Januar 2007 16:47 schrieb Alan Stern:
> On Tue, 2 Jan 2007, Oliver Neukum wrote:
> 
> > Am Dienstag, 2. Januar 2007 16:26 schrieb Alan Stern:
> > > On Tue, 2 Jan 2007, Oliver Neukum wrote:
> > > 
> > > > Hi,
> > > > 
> > > > if a driver returns an error in fill_read_buffer(), the buffer will be
> > > > marked as filled. Subsequent reads will return eof. But there is
> > > > no data because of an error, not because it has been read.
> > > > Not marking the buffer filled is the obvious fix.
> > > > 
> > > > 	Regards
> > > > 		Oliver
> > > > 
> > > > Signed-off-by: Oliver Neukum <oliver@neukum.name>
> > > > --
> > > > 
> > > > --- a/fs/sysfs/file.c	2006-12-24 05:00:32.000000000 +0100
> > > > +++ b/fs/sysfs/file.c	2007-01-01 15:03:14.000000000 +0100
> > > > @@ -70,7 +70,8 @@
> > > >   *	Allocate @buffer->page, if it hasn't been already, then call the
> > > >   *	kobject's show() method to fill the buffer with this attribute's 
> > > >   *	data. 
> > > > - *	This is called only once, on the file's first read. 
> > > > + *	This is called only once, on the file's first read unless an error
> > > > + *	is returned.
> > > >   */
> > > 
> > > I don't think this matches what people expect of sysfs.  If a show method 
> > > fails then the assumption is that the file cannot be read at all, so 
> > > there's no point in trying to call the method again.
> > 
> > This would make handling ENOMEM very hard.
> 
> No harder than handling any other error: Close the sysfs file, then open 
> it and try to read it again.

If you close it, it doesn't matter. However if not, it does.
Not all the world simply uses "cat".

	Regards
		Oliver