Re: [Qemu-trivial] [Qemu-devel] [PATCH] char: fix repeated registration of tcp chardev I/O handlers

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 08/02/16 13:55, Daniel P. Berrange wrote:

> In previous commit:
> 
>   commit f2001a7e0555b66d6db25a3ff1801540814045bb
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Tue Jan 19 11:14:30 2016 +0000
> 
>     char: don't assume telnet initialization will not block
> 
> The code which writes the telnet initialization sequence moved
> to an event loop callback. If the TCP chardev is opened as a
> server in blocking mode (ie -serial telnet:0.0.0.0:3000,server,wait)
> this results in a state where the TCP chardev is connected, but not
> yet ready to send/recv data when virtual hardware is created.
> 
> When the virtual hardware initialization registers its chardev
> callbacks, it triggers tcp_chr_update_read_handler, which will
> add I/O watches to the connection.
> 
> When the telnet initialization finally runs, it will then call
> tcp_chr_connect to finish the connection setup. This will in
> turn add I/O watches to the connection too.
> 
> There are now two sets of I/O watches registered on the same
> connection. This ultimately causes data loss on the connection,
> for example, when typing into the telnet console only every
> second byte is echoed back to the client.
> 
> The same flaw can affect channels running with TLS encryption
> too, since they also have delayed connection setup completion.
> 
> The fix is to update tcp_chr_update_read_handler so that it
> avoids registering watches if the connection is not fully
> setup yet.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-char.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 927c47e..9060f8a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2856,6 +2856,10 @@ static void tcp_chr_update_read_handler(CharDriverState *chr)
>  {
>      TCPCharDriver *s = chr->opaque;
>  
> +    if (!s->connected) {
> +        return;
> +    }
> +
>      remove_fd_in_watch(chr);
>      if (s->ioc) {
>          chr->fd_in_tag = io_add_watch_poll(s->ioc,
> 

Hi Daniel,

Yes I can confirm that this patch fixes the issue for me - thanks a lot
for debugging!


ATB,

Mark.