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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

On Thu, Aug 17, 2023 at 5:06 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> 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.

Indeed. But isn't it possible to watch for IO_OUT and get rid of the timer?

Other thing I question is whether the serial shouldn't return BUSY if
the chardev is disconnected..