Re: [PATCH 5/6] lirc_zilog: error out if buffer read bytes != chunk size

[Jarod Wilson <jarod@redhat.com>]

On Thu, Mar 17, 2011 at 11:29:08AM -0400, Andy Walls wrote:
> Jarod Wilson <jarod@redhat.com> wrote:
> 
> >On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote:
> >> On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote:
> >> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> >> > ---
> >> >  drivers/staging/lirc/lirc_zilog.c |    4 ++++
> >> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/drivers/staging/lirc/lirc_zilog.c
> >b/drivers/staging/lirc/lirc_zilog.c
> >> > index 407d4b4..5ada643 100644
> >> > --- a/drivers/staging/lirc/lirc_zilog.c
> >> > +++ b/drivers/staging/lirc/lirc_zilog.c
> >> > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char
> >*outbuf, size_t n, loff_t *ppos)
> >> >  				ret = copy_to_user((void *)outbuf+written, buf,
> >> >  						   rbuf->chunk_size);
> >> >  				written += rbuf->chunk_size;
> >> > +			} else {
> >> > +				zilog_error("Buffer read failed!\n");
> >> > +				ret = -EIO;
> >> > +				break;
> >> 
> >> No need to break, just let the non-0 ret value drop you out of the
> >while
> >> loop.
> >
> >Ah, indeed. I think I mindlessly copied what the tests just a few lines
> >above were doing without looking at the actual reason for them. I'll
> >remove that break from the patch here locally.
> >
> >-- 
> >Jarod Wilson
> >jarod@redhat.com
> 
> You might also want to take a look at that test to ensure it doesn't break blocking read() behavior.  (man 2 read). I'm swamped ATM and didn't look too hard.
> 
> It seems odd that the lirc buffer object can have data ready (the first branch of the big if() in the while() loop), and yet the read of that lirc buffer object fails.

Generally, it shouldn't, but lirc_buffer_read uses kfifo underneath, and
in the pre-2.6.33 kfifo implementation, the retval from lirc_buffer_read
(as backported by way of media_build) is always 0, which is of course not
equal to chunk_size. So I think that in current kernels, this should never
trigger, and its partially just a note-to-self that this check will go
sideways when running on an older kernel, but not a bad check to have if
something really does go wrong.

-- 
Jarod Wilson
jarod@redhat.com