Re: socket chardevs: data loss when other end closes connection?

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Oct 18, 2022 at 06:55:08PM +0100, Peter Maydell wrote:
> I've been looking at a (long-standing) bug where an avocado test
> intermittently fails.
> 
> This happens because at the avocado end we write "halt\r" to the
> serial console, which is wired up to a Unix socket; but at the UART
> model we only ever see the 'h' character and no further data.  As far
> as I can tell this happens because Avocado closes the socket and the
> QEMU socket chardev layer loses the last few characters of data that
> the guest hasn't yet read at that point.
> 
> This is what seems to me to be going on:
>  * Avocado writes the data ('halt\r') and closes the socket
>    pretty much immediately afterwards
>  * At the glib layer, the socket is polled, and it gets G_IO_IN
>    and G_IO_HUP, indicating "readable, and also closed"
>  * glib's source dispatch mechanism first calls tcp_chr_read()
>    to handle the G_IO_IN part
>  * tcp_chr_read() reads a single byte (the 'h'), because
>    SocketChardev::max_size is 1 (which in turn is because the
>    device model's can_write function returned 1 to say that's
>    all it can accept for now). So there's still data to be
>    read in future
>  * glib now calls tcp_chr_hup() because of the G_IO_HUP (as part
>    of the same handle-all-the-sources loop)
>  * tcp_chr_hup() calls tcp_chr_disconnect(), which basically
>    frees everything, tells the chardev backend that the connection
>    just closed, etc
>  * the data remaining in the socket to be read after the 'h'
>    is never read
> 
> How is this intended to work? I guess the socket ought to go
> into some kind of "disconnecting" state, but not actually do
> a tcp_chr_disconnect() until all the data has been read via
> tcp_chr_read() and it's finally got an EOF indication back from
> tcp_chr_recv() ?

Right, this is basically broken by (lack of) design right now.

The main problem here is that we're watching the socket twice.
One set of callbacks added with io_add_watch_poll, and then
a second callback added with qio_chanel_create_watch just for
G_IO_HUP.

We need there to be only 1 callback, and when that callback
gets  G_IO_IN, it should *ignore* G_IO_HUP until tcp_chr_recv
returns 0 to indicate EOF. This would cause tcp_chr_read to
be invoked repeatedly with G_IO_IN | G_IO_HUP, as we read
"halt\r" one byte at a time.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|