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 <mhart...@linux.ibm.com>
> > > Signed-off-by: Thomas Huth <th...@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 :|
