Re: [Qemu-devel] [PATCH 3/3] hw/char/serial: Don't retry on serial_xmit if errno == EPIPE

[Peter Maydell]

On 1 June 2018 at 13:28, Sergio Lopez <address@hidden> wrote:
> On Fri, Jun 01, 2018 at 02:16:59PM +0200, Paolo Bonzini wrote:
>> On 01/06/2018 14:05, Sergio Lopez wrote:
>> >>> Instead of adding explicit handling of EPIPE, shouldn't the code be
>> >>> rewritten to treat -1 return && errno != EAGAIN as fatal?
>> >> Yes, exactly this code is already broken for every single errno
>> >> value, not simply EPIPE. It needs fixing to treat '-1' return code
>> >> correctly instead of retrying everything.
>> > Given that EAGAIN is already taken care of in
>> > chardev/char.c:qemu_chr_write_buffer, in which cases should we retry? Or
>> > should we just drop all the tsr_retry logic?
>>
>> It's not handled there, the call from qemu_chr_fe_write has write_all ==
>> false.
>
> You're right. So should we just retry only for -1 && errno == EAGAIN and
> just ignore the error otherwise?

I don't think we should make all of qemu_chr_fe_write()'s callers
have to handle EAGAIN. That function already has a way to tell
the caller that it didn't consume any data, which is to return 0.

(EAGAIN is a curse and we should strive to avoid it spreading its
poison into areas of the codebase that we can keep it out of.)

In general there are three cases I think qemu_chr_fe_write() callers
might care about:
 * data was consumed (return >0)
 * data wasn't consumed, try again later (return 0)
 * data wasn't consumed, and a later attempt won't work either
   (return -1)

NB that hw/char/serial.c is not the only serial device using
this pattern and probably needing adjustment (though I think it's
the only one with the complicated tsr_retry logic).

In the patch that started this thread the report is that we
use lots of CPU. Can you explain why that happens? I was expecting
that we would set up the qemu_chr_fe_add_watch() on the dead
chr FE and it would just never fire since the FE can never
accept further data...

thanks
-- PMM