Re: [PATCH] chardev/pty: fix incorrect return value in char_pty_chr_write

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

On Fri, Dec 26, 2025 at 03:55:19PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Dec 25, 2025 at 10:41 AM luzhipeng <luzhipeng@cestc.cn> wrote:
> 
> > In char_pty_chr_write(), when the PTY is not connected (s->connected ==
> > false), the function attempts a non-blocking poll to check if the fd is
> > writable. However, even if the fd is not writable or if io_channel_send()
> > fails the function unconditionally returns 'len', falsely indicating that
> > all data was successfully written.
> >
> > Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
> >
> 
> 
> 
> 
> > ---
> >  chardev/char-pty.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index 652b0bd9e7..37a20d5f4b 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -124,11 +124,15 @@ static int char_pty_chr_write(Chardev *chr, const
> > uint8_t *buf, int len)
> >      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)) {
> > +        return 0;
> > +    }
> >      if (!(pfd.revents & G_IO_HUP) && (pfd.revents & G_IO_OUT)) {
> >          return io_channel_send(s->ioc, buf, len);
> >      }
> >
> > -    return len;
> > +    int ret = io_channel_send(s->ioc, buf, len);
> > +    return ret;
> >
> 
> 
> The 'ret' variable is probably unnecessary.
> 
> I suppose this will return 0 in general (unless the fd state has changed
> after the poll)
> 
> So this will likely conflict the behaviour change from:

I believe behaviour can be worse than the commit below indicates. If we
don't consume data sent by the serial port, then IIRC, wit at least some
serial device impls we'll get a full buffer and hang the guest.

Consider the semantics with a socket based chardev - we'll throw away
all data from the serial port when no client is connected.

We wanted the same behaviour with the PTY. if nothing is connected to
the other end of the PTY, we want to discard all data. Always returning
'len' is achieving that by pretending we wrote the data.

> 
> commit 1c64fdbc8177058802df205f5d7cd65edafa59a8
> Author: Ed Swierk <eswierk@skyportsystems.com>
> Date:   Tue Jan 31 05:45:29 2017 -0800
> 
>     char: drop data written to a disconnected pty
> 
>     When a serial port writes data to a pty that's disconnected, drop the
>     data and return the length dropped. This avoids triggering pointless
>     retries in callers like the 16550A serial_xmit(), and causes
>     qemu_chr_fe_write() to write all data to the log file, rather than
>     logging only while a pty client like virsh console happens to be
>     connected.
> 
>     Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>     Message-Id: <
> 1485870329-79428-1-git-send-email-eswierk@skyportsystems.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 27eb85f505..ecf2c7a5c4 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -129,7 +129,7 @@ static int char_pty_chr_write(Chardev *chr, const
> uint8_t *buf, int len)
>          /* guest sends data, check for (re-)connect */
>          pty_chr_update_read_handler_locked(chr);
>          if (!s->connected) {
> -            return 0;
> +            return len;

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