Re: [PATCH v9 01/16] ui & main loop: Redesign of system-specific main thread event handling
On Mon, 11 Nov 2024 at 05:45, Akihiko Odaki wrote:
>
> On 2024/11/11 6:55, Phil Dennis-Jordan wrote:
> > macOS's Cocoa event handling must be done on the initial (main) thread
> > of the process. Furthermore, if library or application code uses
> > libdispatch, the main dispatch queue must be handling events on the main
> > thread as well.
> >
> > So far, this has affected Qemu in both the Cocoa and SDL UIs, although
> > in different ways: the Cocoa UI replaces the default qemu_main function
> > with one that spins Qemu's internal main event loop off onto a
> > background thread. SDL (which uses Cocoa internally) on the other hand
> > uses a polling approach within Qemu's main event loop. Events are
> > polled during the SDL UI's dpy_refresh callback, which happens to run
> > on the main thread by default.
> >
> > As UIs are mutually exclusive, this works OK as long as nothing else
> > needs platform-native event handling. In the next patch, a new device is
> > introduced based on the ParavirtualizedGraphics.framework in macOS.
> > This uses libdispatch internally, and only works when events are being
> > handled on the main runloop. With the current system, it works when
> > using either the Cocoa or the SDL UI. However, it does not when running
> > headless. Moreover, any attempt to install a similar scheme to the
> > Cocoa UI's main thread replacement fails when combined with the SDL
> > UI.
> >
> > This change tidies up main thread management to be more flexible.
> >
> > * The qemu_main global function pointer is a custom function for the
> > main thread, and it may now be NULL. When it is, the main thread
> > runs the main Qemu loop. This represents the traditional setup.
> > * When non-null, spawning the main Qemu event loop on a separate
> > thread is now done centrally rather than inside the Cocoa UI code.
> > * For most platforms, qemu_main is indeed NULL by default, but on
> > Darwin, it defaults to a function that runs the CFRunLoop.
> > * The Cocoa UI sets qemu_main to a function which runs the
> > NSApplication event handling runloop, as is usual for a Cocoa app.
> > * The SDL UI overrides the qemu_main function to NULL, thus
> > specifying that Qemu's main loop must run on the main
> > thread.
> > * The GTK UI also overrides the qemu_main function to NULL.
> > * For other UIs, or in the absence of UIs, the platform's default
> > behaviour is followed.
> >
> > This means that on macOS, the platform's runloop events are always
> > handled, regardless of chosen UI. The new PV graphics device will
> > thus work in all configurations. There is no functional change on other
> > operating systems.
> >
> > Signed-off-by: Phil Dennis-Jordan
> > Reviewed-by: Akihiko Odaki
> > ---
> > v5:
> >
> > * Simplified the way of setting/clearing the main loop by going back
> > to setting qemu_main directly, but narrowing the scope of what it
> > needs to do, and it can now be NULL.
> >
> > v6:
> >
> > * Folded function qemu_run_default_main_on_new_thread's code into
> > main()
> > * Removed whitespace changes left over on lines near code removed
> > between v4 and v5
> >
> > v9:
> >
> > * Set qemu_main to NULL for GTK UI as well.
> >
> > include/qemu-main.h | 3 +--
> > include/qemu/typedefs.h | 1 +
> > system/main.c | 50 ++
> > ui/cocoa.m | 54 ++---
> > ui/gtk.c| 3 +++
> > ui/sdl2.c | 4 +++
> > 6 files changed, 67 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/qemu-main.h b/include/qemu-main.h
> > index 940960a7dbc..4bd0d667edc 100644
> > --- a/include/qemu-main.h
> > +++ b/include/qemu-main.h
> > @@ -5,7 +5,6 @@
> > #ifndef QEMU_MAIN_H
> > #define QEMU_MAIN_H
> >
> > -int qemu_default_main(void);
> > -extern int (*qemu_main)(void);
> > +extern qemu_main_fn qemu_main;
> >
> > #endif /* QEMU_MAIN_H */
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index 3d84efcac47..b02cfe1f328 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq;
> >* Function types
> >*/
> > typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
> > +typedef int (*qemu_main_fn)(void);
> >
> > #endif /* QEMU_TYPEDEFS_H */
> > diff --git a/system/main.c b/system/main.c
> > index 9b91d21ea8c..d9397a6d5d0 100644
> > --- a/system/main.c
> > +++ b/system/main.c
> > @@ -24,13 +24,14 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu-main.h"
> > +#include "qemu/main-loop.h"
> > #include "sysemu/sysemu.h"
> >
> > -#ifdef CONFIG_SDL
> > -#include
> > +#ifdef CONFIG_DARWIN
> > +#include
> > #endif
> >
> > -int qemu_default_main(void)
> > +static int qemu_default_main(void)
> > {
> > int status;
> >
> > @@ -40,10 +41,49 @@ int qemu_default_main(void)
> > return status;
>
[PATCH] block: Fix leak in send_qmp_error_event
ASAN detected a leak when running the ahci-test
/ahci/io/dma/lba28/retry:
Direct leak of 35 byte(s) in 1 object(s) allocated from:
#0 in malloc
#1 in __vasprintf_internal
#2 in vasprintf
#3 in g_vasprintf
#4 in g_strdup_vprintf
#5 in g_strdup_printf
#6 in object_get_canonical_path ../qom/object.c:2096:19
#7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
#8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
#9 in send_qmp_error_event ../block/block-backend.c:2140:36
#10 in blk_error_action ../block/block-backend.c:2172:9
#11 in ide_handle_rw_error ../hw/ide/core.c:875:5
#12 in ide_dma_cb ../hw/ide/core.c:894:13
#13 in dma_complete ../system/dma-helpers.c:107:9
#14 in dma_blk_cb ../system/dma-helpers.c:129:9
#15 in blk_aio_complete ../block/block-backend.c:1552:9
#16 in blk_aio_write_entry ../block/block-backend.c:1619:5
#17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
Plug the leak by freeing the device path string.
Signed-off-by: Fabiano Rosas
---
block/block-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..a3b7f00188 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2134,13 +2134,14 @@ static void send_qmp_error_event(BlockBackend *blk,
{
IoOperationType optype;
BlockDriverState *bs = blk_bs(blk);
+char *path = blk_get_attached_dev_path(blk);
optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-qapi_event_send_block_io_error(blk_name(blk),
- blk_get_attached_dev_path(blk),
+qapi_event_send_block_io_error(blk_name(blk), path,
bs ? bdrv_get_node_name(bs) : NULL, optype,
action, blk_iostatus_is_enabled(blk),
error == ENOSPC, strerror(error));
+g_free(path);
}
/* This is done by device models because, while the block layer knows
--
2.35.3
Re: [PATCH] block: Fix leak in send_qmp_error_event
Philippe Mathieu-Daudé writes:
> On 11/11/24 14:52, Fabiano Rosas wrote:
>> ASAN detected a leak when running the ahci-test
>> /ahci/io/dma/lba28/retry:
>>
>> Direct leak of 35 byte(s) in 1 object(s) allocated from:
>> #0 in malloc
>> #1 in __vasprintf_internal
>> #2 in vasprintf
>> #3 in g_vasprintf
>> #4 in g_strdup_vprintf
>> #5 in g_strdup_printf
>> #6 in object_get_canonical_path ../qom/object.c:2096:19
>> #7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
>> #8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
>> #9 in send_qmp_error_event ../block/block-backend.c:2140:36
>> #10 in blk_error_action ../block/block-backend.c:2172:9
>> #11 in ide_handle_rw_error ../hw/ide/core.c:875:5
>> #12 in ide_dma_cb ../hw/ide/core.c:894:13
>> #13 in dma_complete ../system/dma-helpers.c:107:9
>> #14 in dma_blk_cb ../system/dma-helpers.c:129:9
>> #15 in blk_aio_complete ../block/block-backend.c:1552:9
>> #16 in blk_aio_write_entry ../block/block-backend.c:1619:5
>> #17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
>>
>> Plug the leak by freeing the device path string.
>>
>> Signed-off-by: Fabiano Rosas
>> ---
>> block/block-backend.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 85bcdedcef..a3b7f00188 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2134,13 +2134,14 @@ static void send_qmp_error_event(BlockBackend *blk,
>> {
>> IoOperationType optype;
>> BlockDriverState *bs = blk_bs(blk);
>> +char *path = blk_get_attached_dev_path(blk);
>
> Preferably using g_autofree,
Of course, I'll repost. Thanks!
Re: [PATCH v8 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote: On Mon, 11 Nov 2024 at 13:41, BALATON Zoltan wrote: On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote: On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé wrote: On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote: On Sun 10. Nov 2024 at 08:01, Akihiko Odaki wrote: On 2024/11/08 23:46, Phil Dennis-Jordan wrote: macOS's Cocoa event handling must be done on the initial (main) thread of the process. Furthermore, if library or application code uses libdispatch, the main dispatch queue must be handling events on the main thread as well. So far, this has affected Qemu in both the Cocoa and SDL UIs, although in different ways: the Cocoa UI replaces the default qemu_main function with one that spins Qemu's internal main event loop off onto a background thread. SDL (which uses Cocoa internally) on the other hand uses a polling approach within Qemu's main event loop. Events are polled during the SDL UI's dpy_refresh callback, which happens to run on the main thread by default. GTK should also do the same as SDL and requires treatment; I forgot to note that in previous reviews. Although it‘s possible to build Qemu with GTK support enabled on macOS, that UI doesn’t actually work on macOS at all, and apparently hasn’t been supported since 2018, see: https://stackoverflow.com/a/51474795 I don’t think there’s any point making adjustments to the GTK code by guessing what might be needed if someone did fix that to work with macOS at some point. If we don't support GTK on macOS, then we should update meson.build to actively prevent users enabling GTK on macOS builds, rather than letting them suffer random runtime crashes. Agreed - I'm now more confused than ever though because https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that Philippe has previously been using it successfully. Or perhaps this was created in response to https://gitlab.com/qemu-project/qemu/-/issues/2515 ? But it seems like even the SDL implementation isn't perfect: https://gitlab.com/qemu-project/qemu/-/issues/2537 Basically, it seems like Qemu's UI threading on macOS is currently a bit of a mess, except in the Cocoa UI. Maybe it worked with older MacOS X releases but broke around the same time when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs a similar fix or whatever cocoa was changed to use since somewhere in gtk or QEMU? Possible! Calling the Cocoa UI APIs from anything other than the main thread has never officially been supported as far as I'm aware, but perhaps some subset silently worked on earlier releases. Modern versions definitely enforce the rule to some extent. Also I find it strange to require UI backends to NULL init a global. If the cocoa UI is the only one that needs it could that also set it instead of doing it in /system? That would also confine macOS specific code to cocoa.m and the other UIs would not need any change that way. Well, that's the whole point, it's not just the Cocoa UI - other macOS system frameworks also need a runloop or libdispatch to be handling events on thread 0. Relevant here are the ParavirtualizedGraphics.framework that's integrated with patches 2-4 from this patch set, as well as the Metal.framework, which PVG uses internally, and which we need to use directly to some extent to complete the generated guest frames. These frameworks internally use libdispatch, and they just don't work if nothing is processing events on the main dispatch queue/runloop. So without patch 01/16, PV graphics will only work in conjunction with Cocoa or SDL, because those do process the runloop on the main thread. But if you were to run with -nographic, or VNC/SPICE-only, Mac PV graphics just wouldn't work. So the idea is to uncouple the main thread's runloop from the Cocoa UI. OK, I think I got that now. An then, the GTK+ and SDL libraries themselves call down into Cocoa on macOS. Windows also has specific rules about its Win32 UI API and threading. SDL says outright that everything needs to be done from the main thread. GTK+ says everything needs to be called from a single thread on Win32; it seems like the rule on macOS is the same, except that thread must be thread 0. Both those UIs in Qemu seem to violate the threading rules of the libraries they integrate, as evidenced by the bug reports listed above. This brokenness is entirely independent of this patch set here, it's just that we're bumping up against the same underlying issue of needing runloop handling on thread 0 in macOS. With regard to NULL'ing the qemu_main function pointer from the SDL or GTK UIs in Qemu: I had implemented a different approach to solving the problem in v4, where each UI declared up-front what kind of threading arrangement it needed rather than each one just overwriting a global variable. This was somewhat more complex than the current one though. https://patchew.org/QEMU/20241024102813.9855-1-p...@
Re: [PATCH v8 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Mon, 11 Nov 2024 at 13:41, BALATON Zoltan wrote: > On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote: > > On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé > > wrote: > > > >> On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote: > >>> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki > >>> wrote: > >>> > On 2024/11/08 23:46, Phil Dennis-Jordan wrote: > > macOS's Cocoa event handling must be done on the initial (main) > >> thread > > of the process. Furthermore, if library or application code uses > > libdispatch, the main dispatch queue must be handling events on the > >> main > > thread as well. > > > > So far, this has affected Qemu in both the Cocoa and SDL UIs, > >> although > > in different ways: the Cocoa UI replaces the default qemu_main > >> function > > with one that spins Qemu's internal main event loop off onto a > > background thread. SDL (which uses Cocoa internally) on the other > >> hand > > uses a polling approach within Qemu's main event loop. Events are > > polled during the SDL UI's dpy_refresh callback, which happens to run > > on the main thread by default. > > GTK should also do the same as SDL and requires treatment; I forgot to > note that in previous reviews. > >>> > >>> > >>> Although it‘s possible to build Qemu with GTK support enabled on macOS, > >>> that UI doesn’t actually work on macOS at all, and apparently hasn’t > been > >>> supported since 2018, see: > >>> https://stackoverflow.com/a/51474795 > >>> > >>> I don’t think there’s any point making adjustments to the GTK code by > >>> guessing what might be needed if someone did fix that to work with > macOS > >> at > >>> some point. > >> > >> If we don't support GTK on macOS, then we should update meson.build > >> to actively prevent users enabling GTK on macOS builds, rather than > >> letting them suffer random runtime crashes. > >> > >> > > Agreed - I'm now more confused than ever though because > > https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that > > Philippe has previously been using it successfully. Or perhaps this was > > created in response to > https://gitlab.com/qemu-project/qemu/-/issues/2515 ? > > But it seems like even the SDL implementation isn't perfect: > > https://gitlab.com/qemu-project/qemu/-/issues/2537 > > > > Basically, it seems like Qemu's UI threading on macOS is currently a bit > of > > a mess, except in the Cocoa UI. > > Maybe it worked with older MacOS X releases but broke around the same time > when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs > a similar fix or whatever cocoa was changed to use since somewhere in gtk > or QEMU? > Possible! Calling the Cocoa UI APIs from anything other than the main thread has never officially been supported as far as I'm aware, but perhaps some subset silently worked on earlier releases. Modern versions definitely enforce the rule to some extent. Also I find it strange to require UI backends to NULL init a global. If > the cocoa UI is the only one that needs it could that also set it instead > of doing it in /system? That would also confine macOS specific code to > cocoa.m and the other UIs would not need any change that way. > Well, that's the whole point, it's not just the Cocoa UI - other macOS system frameworks also need a runloop or libdispatch to be handling events on thread 0. Relevant here are the ParavirtualizedGraphics.framework that's integrated with patches 2-4 from this patch set, as well as the Metal.framework, which PVG uses internally, and which we need to use directly to some extent to complete the generated guest frames. These frameworks internally use libdispatch, and they just don't work if nothing is processing events on the main dispatch queue/runloop. So without patch 01/16, PV graphics will only work in conjunction with Cocoa or SDL, because those do process the runloop on the main thread. But if you were to run with -nographic, or VNC/SPICE-only, Mac PV graphics just wouldn't work. So the idea is to uncouple the main thread's runloop from the Cocoa UI. An then, the GTK+ and SDL libraries themselves call down into Cocoa on macOS. Windows also has specific rules about its Win32 UI API and threading. SDL says outright that everything needs to be done from the main thread. GTK+ says everything needs to be called from a single thread on Win32; it seems like the rule on macOS is the same, except that thread must be thread 0. Both those UIs in Qemu seem to violate the threading rules of the libraries they integrate, as evidenced by the bug reports listed above. This brokenness is entirely independent of this patch set here, it's just that we're bumping up against the same underlying issue of needing runloop handling on thread 0 in macOS. With regard to NULL'ing the qemu_main function pointer from the SDL or GTK UIs in Qemu: I had implemented a different approach to solving the problem in v4, where each UI declared up-f
Re: [PATCH v8 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé wrote: > On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote: > > On Sun 10. Nov 2024 at 08:01, Akihiko Odaki > > wrote: > > > > > On 2024/11/08 23:46, Phil Dennis-Jordan wrote: > > > > macOS's Cocoa event handling must be done on the initial (main) > thread > > > > of the process. Furthermore, if library or application code uses > > > > libdispatch, the main dispatch queue must be handling events on the > main > > > > thread as well. > > > > > > > > So far, this has affected Qemu in both the Cocoa and SDL UIs, > although > > > > in different ways: the Cocoa UI replaces the default qemu_main > function > > > > with one that spins Qemu's internal main event loop off onto a > > > > background thread. SDL (which uses Cocoa internally) on the other > hand > > > > uses a polling approach within Qemu's main event loop. Events are > > > > polled during the SDL UI's dpy_refresh callback, which happens to run > > > > on the main thread by default. > > > > > > GTK should also do the same as SDL and requires treatment; I forgot to > > > note that in previous reviews. > > > > > > Although it‘s possible to build Qemu with GTK support enabled on macOS, > > that UI doesn’t actually work on macOS at all, and apparently hasn’t been > > supported since 2018, see: > > https://stackoverflow.com/a/51474795 > > > > I don’t think there’s any point making adjustments to the GTK code by > > guessing what might be needed if someone did fix that to work with macOS > at > > some point. > > If we don't support GTK on macOS, then we should update meson.build > to actively prevent users enabling GTK on macOS builds, rather than > letting them suffer random runtime crashes. > > Agreed - I'm now more confused than ever though because https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that Philippe has previously been using it successfully. Or perhaps this was created in response to https://gitlab.com/qemu-project/qemu/-/issues/2515 ? But it seems like even the SDL implementation isn't perfect: https://gitlab.com/qemu-project/qemu/-/issues/2537 Basically, it seems like Qemu's UI threading on macOS is currently a bit of a mess, except in the Cocoa UI.
Re: [PATCH v8 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote: > On Sun 10. Nov 2024 at 08:01, Akihiko Odaki > wrote: > > > On 2024/11/08 23:46, Phil Dennis-Jordan wrote: > > > macOS's Cocoa event handling must be done on the initial (main) thread > > > of the process. Furthermore, if library or application code uses > > > libdispatch, the main dispatch queue must be handling events on the main > > > thread as well. > > > > > > So far, this has affected Qemu in both the Cocoa and SDL UIs, although > > > in different ways: the Cocoa UI replaces the default qemu_main function > > > with one that spins Qemu's internal main event loop off onto a > > > background thread. SDL (which uses Cocoa internally) on the other hand > > > uses a polling approach within Qemu's main event loop. Events are > > > polled during the SDL UI's dpy_refresh callback, which happens to run > > > on the main thread by default. > > > > GTK should also do the same as SDL and requires treatment; I forgot to > > note that in previous reviews. > > > Although it‘s possible to build Qemu with GTK support enabled on macOS, > that UI doesn’t actually work on macOS at all, and apparently hasn’t been > supported since 2018, see: > https://stackoverflow.com/a/51474795 > > I don’t think there’s any point making adjustments to the GTK code by > guessing what might be needed if someone did fix that to work with macOS at > some point. If we don't support GTK on macOS, then we should update meson.build to actively prevent users enabling GTK on macOS builds, rather than letting them suffer random runtime crashes. 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 :|
Re: [PATCH v8 01/15] ui & main loop: Redesign of system-specific main thread event handling
On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote: On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé wrote: On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote: On Sun 10. Nov 2024 at 08:01, Akihiko Odaki wrote: On 2024/11/08 23:46, Phil Dennis-Jordan wrote: macOS's Cocoa event handling must be done on the initial (main) thread of the process. Furthermore, if library or application code uses libdispatch, the main dispatch queue must be handling events on the main thread as well. So far, this has affected Qemu in both the Cocoa and SDL UIs, although in different ways: the Cocoa UI replaces the default qemu_main function with one that spins Qemu's internal main event loop off onto a background thread. SDL (which uses Cocoa internally) on the other hand uses a polling approach within Qemu's main event loop. Events are polled during the SDL UI's dpy_refresh callback, which happens to run on the main thread by default. GTK should also do the same as SDL and requires treatment; I forgot to note that in previous reviews. Although it‘s possible to build Qemu with GTK support enabled on macOS, that UI doesn’t actually work on macOS at all, and apparently hasn’t been supported since 2018, see: https://stackoverflow.com/a/51474795 I don’t think there’s any point making adjustments to the GTK code by guessing what might be needed if someone did fix that to work with macOS at some point. If we don't support GTK on macOS, then we should update meson.build to actively prevent users enabling GTK on macOS builds, rather than letting them suffer random runtime crashes. Agreed - I'm now more confused than ever though because https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that Philippe has previously been using it successfully. Or perhaps this was created in response to https://gitlab.com/qemu-project/qemu/-/issues/2515 ? But it seems like even the SDL implementation isn't perfect: https://gitlab.com/qemu-project/qemu/-/issues/2537 Basically, it seems like Qemu's UI threading on macOS is currently a bit of a mess, except in the Cocoa UI. Maybe it worked with older MacOS X releases but broke around the same time when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs a similar fix or whatever cocoa was changed to use since somewhere in gtk or QEMU? Also I find it strange to require UI backends to NULL init a global. If the cocoa UI is the only one that needs it could that also set it instead of doing it in /system? That would also confine macOS specific code to cocoa.m and the other UIs would not need any change that way. Regards, BALATON Zoltan
Re: [PATCH] block: Fix leak in send_qmp_error_event
On 11/11/24 14:52, Fabiano Rosas wrote:
ASAN detected a leak when running the ahci-test
/ahci/io/dma/lba28/retry:
Direct leak of 35 byte(s) in 1 object(s) allocated from:
#0 in malloc
#1 in __vasprintf_internal
#2 in vasprintf
#3 in g_vasprintf
#4 in g_strdup_vprintf
#5 in g_strdup_printf
#6 in object_get_canonical_path ../qom/object.c:2096:19
#7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
#8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
#9 in send_qmp_error_event ../block/block-backend.c:2140:36
#10 in blk_error_action ../block/block-backend.c:2172:9
#11 in ide_handle_rw_error ../hw/ide/core.c:875:5
#12 in ide_dma_cb ../hw/ide/core.c:894:13
#13 in dma_complete ../system/dma-helpers.c:107:9
#14 in dma_blk_cb ../system/dma-helpers.c:129:9
#15 in blk_aio_complete ../block/block-backend.c:1552:9
#16 in blk_aio_write_entry ../block/block-backend.c:1619:5
#17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
Plug the leak by freeing the device path string.
Signed-off-by: Fabiano Rosas
---
block/block-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..a3b7f00188 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2134,13 +2134,14 @@ static void send_qmp_error_event(BlockBackend *blk,
{
IoOperationType optype;
BlockDriverState *bs = blk_bs(blk);
+char *path = blk_get_attached_dev_path(blk);
Preferably using g_autofree,
Reviewed-by: Philippe Mathieu-Daudé
optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-qapi_event_send_block_io_error(blk_name(blk),
- blk_get_attached_dev_path(blk),
+qapi_event_send_block_io_error(blk_name(blk), path,
bs ? bdrv_get_node_name(bs) : NULL, optype,
action, blk_iostatus_is_enabled(blk),
error == ENOSPC, strerror(error));
+g_free(path);
}
/* This is done by device models because, while the block layer knows
Having read this patch, we should improve the doc for this methods.
I'll post the following later:
-- >8 --
diff --git a/include/sysemu/block-backend-io.h
b/include/sysemu/block-backend-io.h
index d174275a5c..ba8dfcc7d0 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -32,6 +32,13 @@ void blk_set_allow_aio_context_change(BlockBackend
*blk, bool allow);
void blk_set_disable_request_queuing(BlockBackend *blk, bool disable);
bool blk_iostatus_is_enabled(const BlockBackend *blk);
+/*
+ * Return the qdev ID, or if no ID is assigned the QOM path,
+ * of the block device attached to the BlockBackend.
+ *
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
char *blk_get_attached_dev_id(BlockBackend *blk);
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..6128012953 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1019,6 +1019,10 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk)
return blk->dev;
}
+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
static char *blk_get_attached_dev_id_or_path(BlockBackend *blk, bool
want_id)
{
DeviceState *dev = blk->dev;
@@ -1033,15 +1037,15 @@ static char
*blk_get_attached_dev_id_or_path(BlockBackend *blk, bool want_id)
return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
}
-/*
- * Return the qdev ID, or if no ID is assigned the QOM path, of the block
- * device attached to the BlockBackend.
- */
char *blk_get_attached_dev_id(BlockBackend *blk)
{
return blk_get_attached_dev_id_or_path(blk, true);
}
+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
static char *blk_get_attached_dev_path(BlockBackend *blk)
{
return blk_get_attached_dev_id_or_path(blk, false);
---
[PATCH v2 1/2] block: Improve blk_get_attached_dev_id() docstring
Expose the method docstring in the header, and mention
returned value must be free'd by caller.
Reported-by: Fabiano Rosas
Signed-off-by: Philippe Mathieu-Daudé
---
include/sysemu/block-backend-io.h | 7 +++
block/block-backend.c | 12
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/block-backend-io.h
b/include/sysemu/block-backend-io.h
index d174275a5c..ba8dfcc7d0 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -32,6 +32,13 @@ void blk_set_allow_aio_context_change(BlockBackend *blk,
bool allow);
void blk_set_disable_request_queuing(BlockBackend *blk, bool disable);
bool blk_iostatus_is_enabled(const BlockBackend *blk);
+/*
+ * Return the qdev ID, or if no ID is assigned the QOM path,
+ * of the block device attached to the BlockBackend.
+ *
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
char *blk_get_attached_dev_id(BlockBackend *blk);
BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..7b329ff194 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1019,6 +1019,10 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk)
return blk->dev;
}
+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
static char *blk_get_attached_dev_id_or_path(BlockBackend *blk, bool want_id)
{
DeviceState *dev = blk->dev;
@@ -1033,15 +1037,15 @@ static char
*blk_get_attached_dev_id_or_path(BlockBackend *blk, bool want_id)
return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
}
-/*
- * Return the qdev ID, or if no ID is assigned the QOM path, of the block
- * device attached to the BlockBackend.
- */
char *blk_get_attached_dev_id(BlockBackend *blk)
{
return blk_get_attached_dev_id_or_path(blk, true);
}
+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
static char *blk_get_attached_dev_path(BlockBackend *blk)
{
return blk_get_attached_dev_id_or_path(blk, false);
--
2.45.2
[PATCH v2 0/2] block: Fix leak in send_qmp_error_event
Respin of Fabiano patch using g_autofree, and clarifying method docstrings. Fabiano Rosas (1): block: Fix leak in send_qmp_error_event Philippe Mathieu-Daudé (1): block: Improve blk_get_attached_dev_id() docstring include/sysemu/block-backend-io.h | 7 +++ block/block-backend.c | 16 ++-- 2 files changed, 17 insertions(+), 6 deletions(-) -- 2.45.2
[PATCH v2 2/2] block: Fix leak in send_qmp_error_event
From: Fabiano Rosas
ASAN detected a leak when running the ahci-test
/ahci/io/dma/lba28/retry:
Direct leak of 35 byte(s) in 1 object(s) allocated from:
#0 in malloc
#1 in __vasprintf_internal
#2 in vasprintf
#3 in g_vasprintf
#4 in g_strdup_vprintf
#5 in g_strdup_printf
#6 in object_get_canonical_path ../qom/object.c:2096:19
#7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
#8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
#9 in send_qmp_error_event ../block/block-backend.c:2140:36
#10 in blk_error_action ../block/block-backend.c:2172:9
#11 in ide_handle_rw_error ../hw/ide/core.c:875:5
#12 in ide_dma_cb ../hw/ide/core.c:894:13
#13 in dma_complete ../system/dma-helpers.c:107:9
#14 in dma_blk_cb ../system/dma-helpers.c:129:9
#15 in blk_aio_complete ../block/block-backend.c:1552:9
#16 in blk_aio_write_entry ../block/block-backend.c:1619:5
#17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
Plug the leak by freeing the device path string.
Signed-off-by: Fabiano Rosas
Reviewed-by: Philippe Mathieu-Daudé
Message-ID: <2024145214.8261-1-faro...@suse.de>
[PMD: Use g_autofree]
Signed-off-by: Philippe Mathieu-Daudé
---
block/block-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 7b329ff194..6128012953 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2138,10 +2138,10 @@ static void send_qmp_error_event(BlockBackend *blk,
{
IoOperationType optype;
BlockDriverState *bs = blk_bs(blk);
+g_autofree char *path = blk_get_attached_dev_path(blk);
optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-qapi_event_send_block_io_error(blk_name(blk),
- blk_get_attached_dev_path(blk),
+qapi_event_send_block_io_error(blk_name(blk), path,
bs ? bdrv_get_node_name(bs) : NULL, optype,
action, blk_iostatus_is_enabled(blk),
error == ENOSPC, strerror(error));
--
2.45.2
