Re: [Qemu-devel] [libvirt] char: Logging serial pty output when disconnected

[Paolo Bonzini <pbonzini@redhat.com>]

On 31/01/2017 04:39, Daniel P. Berrange wrote:
> On Mon, Jan 30, 2017 at 08:58:25PM -0800, Ed Swierk wrote:
>> On Fri, Jan 27, 2017 at 1:40 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
>>> On Thu, Jan 26, 2017 at 05:07:16PM -0800, Ed Swierk wrote:
>>>> Currently qemu_chr_fe_write() calls qemu_chr_fe_write_log() only for
>>>> data consumed by the backend chr_write function. With the pty backend,
>>>> pty_chr_write() returns 0 indicating that the data was not consumed
>>>> when the pty is disconnected. Simply changing it to return len instead
>>>> of 0 tricks the caller into logging the data even when the pty is
>>>> disconnected. I don't know what problems this might cause, but one
>>>> data point is that tcp_chr_write() already happens to work this way.
>>>>
>>>> Alternatively, qemu_chr_fe_write() could be modified to log everything
>>>> passed to it, regardless of how much data chr_write claims to have
>>>> consumed. The trouble is that the serial device retries writing
>>>> unconsumed data, so when the pty is disconnected you'd see every
>>>> character duplicated 4 times in the log file.
>>>>
>>>> Any opinions on either approach, or other suggestions? If there are no
>>>> objections to the first one, I'll prepare a patch.
>>>
>>> If the pty backend intends to just drop data into a blackhole when
>>> no client is connected, then its chr_write() impl should return
>>> the length of the data discarded, not zero.
>>
>> That's exactly the question: when no client is connected, should the
>> pty backend just drop the data into a black hole, returning the length
>> of the data discarded? Or should it return 0, letting the frontend
>> device decide what to do with it?
> 
> It should return len of data discarded.
> 
>>
>> I can't discern a consistent pattern across all the char backends. The
>> closest analog is the tcp backend, which does discard the data and
>> return len. In contrast, several backends call
>> io_channel_send{,_full}(), which returns -1 if the write would block
>> or fails for any other reason.
>>
>> It's not clear there's much the frontend can do to recover from an
>> error, but there's no consistent pattern across serial devices either.
>> Most just ignore the return value. But the 16550A serial device
>> retries 4 times after an error. Changing the pty backend to discard
>> the data on the first attempt would bypass this retry mechanism. Is
>> that a problem?
> 
> I don't think so - retrying in this way is pointless IMHO - it is just
> going to get the same result on every retry on 99% of occassions.

Just to provide the full context, the retry happens even if you get
EAGAIN, and in that case it does makes sense.

But if the pty is disconnected I agree it should discard the data.

Paolo