[v2] chardev: Improve @docstring and clarify qemu_chr_write() uses

[PATCH v2 5/9] chardev/char-pty: Do not ignore chr_write() failures

Posted by Philippe Mathieu-Daudé 3 weeks, 2 days ago

Cc: qemu-stable@nongnu.org
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
If ignoring this is deliberate, this must be described in a comment
to avoid any confusion.
---
 chardev/char-pty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index b066f014126..652b0bd9e73 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -125,7 +125,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
     rc = RETRY_ON_EINTR(g_poll(&pfd, 1, 0));
     g_assert(rc >= 0);
     if (!(pfd.revents & G_IO_HUP) && (pfd.revents & G_IO_OUT)) {
-        io_channel_send(s->ioc, buf, len);
+        return io_channel_send(s->ioc, buf, len);
     }
 
     return len;
-- 
2.51.0

Re: [PATCH v2 5/9] chardev/char-pty: Do not ignore chr_write() failures

Posted by Marc-André Lureau 2 weeks, 3 days ago

Hi

On Wed, Oct 22, 2025 at 7:09 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> If ignoring this is deliberate, this must be described in a comment
> to avoid any confusion.

Agree
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

This is from commit 4f7689f08 ("chardev/char-pty: Avoid losing bytes
when the other side just (re-)connected")

note: It's mildly annoying that the pty backend has still
!s->connected and we are writing. I wonder why the frontend is not
respecting the backend opened state, probably some race or buffering.


> ---
>  chardev/char-pty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index b066f014126..652b0bd9e73 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -125,7 +125,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
>      rc = RETRY_ON_EINTR(g_poll(&pfd, 1, 0));
>      g_assert(rc >= 0);
>      if (!(pfd.revents & G_IO_HUP) && (pfd.revents & G_IO_OUT)) {
> -        io_channel_send(s->ioc, buf, len);
> +        return io_channel_send(s->ioc, buf, len);
>      }
>
>      return len;
> --
> 2.51.0
>
>


-- 
Marc-André Lureau

Re: [PATCH v2 5/9] chardev/char-pty: Do not ignore chr_write() failures

Posted by Philippe Mathieu-Daudé 2 days, 6 hours ago

On 28/10/25 14:44, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Oct 22, 2025 at 7:09 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> If ignoring this is deliberate, this must be described in a comment
>> to avoid any confusion.
> 
> Agree
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

> 
> This is from commit 4f7689f08 ("chardev/char-pty: Avoid losing bytes
> when the other side just (re-)connected")
> 
> note: It's mildly annoying that the pty backend has still
> !s->connected and we are writing.

Per the comment, "already re-connected, but timer not fired yet".

> I wonder why the frontend is not
> respecting the backend opened state, probably some race or buffering.

Do you mind opening an issue? I'm not familiar with this code.

> 
> 
>> ---
>>   chardev/char-pty.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index b066f014126..652b0bd9e73 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -125,7 +125,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
>>       rc = RETRY_ON_EINTR(g_poll(&pfd, 1, 0));
>>       g_assert(rc >= 0);
>>       if (!(pfd.revents & G_IO_HUP) && (pfd.revents & G_IO_OUT)) {
>> -        io_channel_send(s->ioc, buf, len);
>> +        return io_channel_send(s->ioc, buf, len);
>>       }
>>
>>       return len;
>> --
>> 2.51.0
>>
>>
> 
>