Re: [PATCH] chardev/char-pty: Avoid losing bytes when the other side just (re-)connected

[Daniel P . Berrangé]

On Thu, Aug 17, 2023 at 02:00:26PM +0200, Thomas Huth wrote:
> On 17/08/2023 12.32, Daniel P. Berrangé wrote:
> > On Wed, Aug 16, 2023 at 11:07:43PM +0200, Thomas Huth wrote:
> > > When starting a guest via libvirt with "virsh start --console ...",
> > > the first second of the console output is missing. This is especially
> > > annoying on s390x that only has a text console by default and no graphical
> > > output - if the bios fails to boot here, the information about what went
> > > wrong is completely lost.
> > > 
> > > One part of the problem (there is also some things to be done on the
> > > libvirt side) is that QEMU only checks with a 1 second timer whether
> > > the other side of the pty is already connected, so the first second of
> > > the console output is always lost.
> > > 
> > > This likely used to work better in the past, since the code once checked
> > > for a re-connection during write, but this has been removed in commit
> > > f8278c7d74 ("char-pty: remove the check for connection on write") to avoid
> > > some locking.
> > > 
> > > To ease the situation here at least a little bit, let's check with 
> > > g_poll()
> > > whether we could send out the data anyway, even if the connection has not
> > > been marked as "connected" yet. The file descriptor is marked as 
> > > non-blocking
> > > anyway since commit fac6688a18 ("Do not hang on full PTY"), so this should
> > > not cause any trouble if the other side is not ready for receiving yet.
> > > 
> > > With this patch applied, I can now successfully see the bios output of
> > > a s390x guest when running it with "virsh start --console" (with a patched
> > > version of virsh that fixes the remaining issues there, too).
> > > 
> > > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   chardev/char-pty.c | 22 +++++++++++++++++++---
> > >   1 file changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > > index 4e5deac18a..fad12dfef3 100644
> > > --- a/chardev/char-pty.c
> > > +++ b/chardev/char-pty.c
> > > @@ -106,11 +106,27 @@ static void pty_chr_update_read_handler(Chardev 
> > > *chr)
> > >   static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
> > >   {
> > >       PtyChardev *s = PTY_CHARDEV(chr);
> > > +    GPollFD pfd;
> > > +    int rc;
> > > -    if (!s->connected) {
> > > -        return len;
> > > +    if (s->connected) {
> > > +        return io_channel_send(s->ioc, buf, len);
> > >       }
> > > -    return io_channel_send(s->ioc, buf, len);
> > > +
> > > +    /*
> > > +     * The other side might already be re-connected, but the timer might
> > > +     * not have fired yet. So let's check here whether we can write 
> > > again:
> > > +     */
> > > +    pfd.fd = QIO_CHANNEL_FILE(s->ioc)->fd;
> > > +    pfd.events = G_IO_OUT;
> > > +    pfd.revents = 0;
> > > +    rc = RETRY_ON_EINTR(g_poll(&pfd, 1, 0));
> > > +    g_assert(rc >= 0);
> > > +    if (!(pfd.revents & G_IO_HUP) && (pfd.revents & G_IO_OUT)) {
> > 
> > Should (can?) we call
> > 
> >     pty_chr_state(chr, 1);
> > 
> > here ?
> 
> As far as I understood commit f8278c7d74c6 and f7ea2038bea04628, this is not
> possible anymore since the lock has been removed.
> 
> > > +        io_channel_send(s->ioc, buf, len);
> > 
> > As it feels a little dirty to be sending data before setting the
> > 'connected == 1' and thus issuing the 'CHR_EVENT_OPENED' event
> 
> I didn't find a really better solution so far. We could maybe introduce a
> buffer in the char-pty code and store the last second of guest output, but
> IMHO that's way more complex and thus somewhat ugly, too?

The orignal commit f8278c7d74c6 said

[quote]
    char-pty: remove the check for connection on write
    
    This doesn't help much compared to the 1 second poll PTY
    timer. I can't think of a use case where this would help.
[/quote]

We've now identified a use case where it is actually important.

IOW, there's a justification to revert both f7ea2038bea04628 and
f8278c7d74c6, re-adding the locking and write update logic.

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 :|