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 <[email protected]>
--
--- 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;
}
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 <[email protected]>
> --
>
> --- 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
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 <[email protected]>
> > --
> >
> > --- 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
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 <[email protected]>
> > > --
> > >
> > > --- 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
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 <[email protected]>
> > > > --
> > > >
> > > > --- 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