chardev/pty: fix incorrect return value in char_pty_chr_write

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

Posted by luzhipeng 1 month, 1 week ago

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;
 }
 
 static GSource *pty_chr_add_watch(Chardev *chr, GIOCondition cond)
-- 
2.45.1.windows.1

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

Posted by Marc-André Lureau 1 month, 1 week ago

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:

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;

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

Posted by Daniel P. Berrangé 1 month ago

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