qemu_chr_write() dispatches to ChardevClass::chr_write(),
and is expected to propagate the backend error, not some
unrelated one produce by "best effort" logfile or replay.
Preserve and return the relevant %errno.
Cc: qemu-stable@nongnu.org
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
chardev/char.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/chardev/char.c b/chardev/char.c
index 5c8130b2435..2af402d9855 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -113,6 +113,7 @@ static int qemu_chr_write_buffer(Chardev *s,
int *offset, bool write_all)
{
ChardevClass *cc = CHARDEV_GET_CLASS(s);
+ int saved_errno;
int res = 0;
*offset = 0;
@@ -138,6 +139,7 @@ static int qemu_chr_write_buffer(Chardev *s,
break;
}
}
+ saved_errno = errno;
if (*offset > 0) {
/*
* If some data was written by backend, we should
@@ -154,6 +156,7 @@ static int qemu_chr_write_buffer(Chardev *s,
*/
qemu_chr_write_log(s, buf, len);
}
+ errno = saved_errno;
qemu_mutex_unlock(&s->chr_write_lock);
return res;
@@ -186,7 +189,9 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
res = qemu_chr_write_buffer(s, buf, len, &offset, write_all);
if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
+ int saved_errno = errno;
replay_char_write_event_save(res, offset);
+ errno = saved_errno;
}
if (res < 0 && offset == 0) {
--
2.51.0
Hi
On Wed, Oct 22, 2025 at 7:10 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> qemu_chr_write() dispatches to ChardevClass::chr_write(),
> and is expected to propagate the backend error, not some
> unrelated one produce by "best effort" logfile or replay.
> Preserve and return the relevant %errno.
Indeed.. imho we should avoid using errno, it's too easy to clutter.
Even qemu mutex, which may use trace, may change it...
patch lgtm anyway
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> chardev/char.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 5c8130b2435..2af402d9855 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -113,6 +113,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> int *offset, bool write_all)
> {
> ChardevClass *cc = CHARDEV_GET_CLASS(s);
> + int saved_errno;
> int res = 0;
> *offset = 0;
>
> @@ -138,6 +139,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> break;
> }
> }
> + saved_errno = errno;
> if (*offset > 0) {
> /*
> * If some data was written by backend, we should
> @@ -154,6 +156,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> */
> qemu_chr_write_log(s, buf, len);
> }
> + errno = saved_errno;
> qemu_mutex_unlock(&s->chr_write_lock);
>
> return res;
> @@ -186,7 +189,9 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
> res = qemu_chr_write_buffer(s, buf, len, &offset, write_all);
>
> if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
> + int saved_errno = errno;
> replay_char_write_event_save(res, offset);
> + errno = saved_errno;
> }
>
> if (res < 0 && offset == 0) {
> --
> 2.51.0
>
>
--
Marc-André Lureau
On Tue, Oct 28, 2025 at 06:00:33PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Oct 22, 2025 at 7:10 PM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: > > > > qemu_chr_write() dispatches to ChardevClass::chr_write(), > > and is expected to propagate the backend error, not some > > unrelated one produce by "best effort" logfile or replay. > > Preserve and return the relevant %errno. > > Indeed.. imho we should avoid using errno, it's too easy to clutter. > Even qemu mutex, which may use trace, may change it... > > patch lgtm anyway > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Given you say 'qemu mutex, which may use trace, may change it...' then surely this patch is broken.... > > @@ -154,6 +156,7 @@ static int qemu_chr_write_buffer(Chardev *s, > > */ > > qemu_chr_write_log(s, buf, len); > > } > > + errno = saved_errno; > > qemu_mutex_unlock(&s->chr_write_lock); ^^^ This mutex_unlock call may clobber 'errno' that we've just tried to restore. > > > > return res; 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 :|
On 22/10/25 17:07, Philippe Mathieu-Daudé wrote:
> qemu_chr_write() dispatches to ChardevClass::chr_write(),
> and is expected to propagate the backend error, not some
> unrelated one produce by "best effort" logfile or replay.
> Preserve and return the relevant %errno.
>
> Cc: qemu-stable@nongnu.org
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> chardev/char.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 5c8130b2435..2af402d9855 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -113,6 +113,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> int *offset, bool write_all)
> {
> ChardevClass *cc = CHARDEV_GET_CLASS(s);
> + int saved_errno;
> int res = 0;
> *offset = 0;
>
> @@ -138,6 +139,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> break;
> }
> }
> + saved_errno = errno;
> if (*offset > 0) {
> /*
> * If some data was written by backend, we should
> @@ -154,6 +156,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> */
> qemu_chr_write_log(s, buf, len);
> }
> + errno = saved_errno;
> qemu_mutex_unlock(&s->chr_write_lock);
>
> return res;
> @@ -186,7 +189,9 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all)
> res = qemu_chr_write_buffer(s, buf, len, &offset, write_all);
>
> if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
> + int saved_errno = errno;
> replay_char_write_event_save(res, offset);
> + errno = saved_errno;
> }
>
> if (res < 0 && offset == 0) {
On Wed, 22 Oct 2025 at 16:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 22/10/25 17:07, Philippe Mathieu-Daudé wrote: > > qemu_chr_write() dispatches to ChardevClass::chr_write(), > > and is expected to propagate the backend error, not some > > unrelated one produce by "best effort" logfile or replay. > > Preserve and return the relevant %errno. > > > > Cc: qemu-stable@nongnu.org > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> ...did I? I remember being confused by the errno usage in these functions but I don't remember what I thought was the best way to untangle it... -- PMM
On 28/10/25 15:25, Peter Maydell wrote: > On Wed, 22 Oct 2025 at 16:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 22/10/25 17:07, Philippe Mathieu-Daudé wrote: >>> qemu_chr_write() dispatches to ChardevClass::chr_write(), >>> and is expected to propagate the backend error, not some >>> unrelated one produce by "best effort" logfile or replay. >>> Preserve and return the relevant %errno. >>> >>> Cc: qemu-stable@nongnu.org >> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > ...did I? I remember being confused by the errno usage > in these functions but I don't remember what I thought > was the best way to untangle it... In March when reviewing PL011 TX FIFO patches ;) https://lore.kernel.org/qemu-devel/CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/
© 2016 - 2025 Red Hat, Inc.