Dne 30.12.2017 (sob) ob 17:22 +0200 je Tanu Kaskinen napisal(a):
> On Sat, 2017-12-30 at 14:00 +0100, Samo PogaÄ?nik wrote:
> >
> > Dne 29.12.2017 (pet) ob 15:40 +0200 je Tanu Kaskinen napisal(a):
> > >
> > > On Sat, 2017-12-16 at 16:57 +0100, Samo PogaÄ?nik wrote:
> > > >
> > > >
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â }
> > > > +Â Â Â Â Â Â Â Â } else {
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â count += l;
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â index += l;
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â length -= l;
> > > > +
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â if (length <= 0) {
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â }
> > > > +Â Â Â Â Â Â Â Â }
> > > > +Â Â Â Â }
> > > > +
> > > > +Â Â Â Â pa_memblock_release(pchunk->memblock);
> > > > +
> > > > +Â Â Â Â if (u->fifo_error && count >= 0) {
> > > > +Â Â Â Â Â Â Â Â pa_log("Recovered from FIFO error");
> > > > +Â Â Â Â Â Â Â Â u->fifo_error = false;
> > > > +Â Â Â Â }
> > > This logic is not right. If in the first loop iteration
> > > pa_write()
> > > does
> > > a partial write and then an error occurs during the second
> > > iteration,
> > > the code incorrectly thinks that we have recovered from the
> > > error.
> > > The
> > > recovery happens when pa_write() returns a positive number for
> > > the
> > > first time.
> > >
> > This case is correctly covered, since count always get negative
> > value
> > in the case of a fifo error, so immediate recovery can not occure
> > via
> > this condition.
> Ah, indeed. My mistake.
>
> I can still think of an (unlikely) case that doesn't work quite
> right,
> though:
>
> Let's say an error has occurred and fifo_error is true. Then
> pipe_sink_write() is called, and the first pa_write() succeeds, but
> does only a partial write. The error should be considered recovered
> at
> this point, but your code doesn't set fifo_error = false. When trying
> to write the rest of the chunk, an error happens. Now an error
> message
> should be logged, but since fifo_error is still true, that doesn't
> happen.
>
That is true. If you agree, i'll move/change the recovery condition
into the success block of the pa_write() return check like this:
...
    } else {
--> Â Â Â Â if (u->fifo_error) {
--> Â Â Â Â Â Â pa_log("Recovered from FIFO error");
--> Â Â Â Â Â Â u->fifo_error = false;
--> Â Â Â Â }
            count += l;
            index += l;
            length -= l;
...
> >Â
> > +Â Â Â Â return count;
> > +}
> > +
> > +static int process_render_use_timing(struct userdata *u, pa_usec_t
> > now) {
> The function return type can be changed to void.
>Â
Can switch return type back to void - i just left the code prepared for
potential error case, that would require full pipe_sink failure with
module unloading.
What is your comment?
regards,
Samo
