| Message ID | 20230913144959.41891-1-lersek@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ui/console: multihead: fix crash, simplify logic | expand |
Hi Laszlo On Wed, Sep 13, 2023 at 6:50 PM Laszlo Ersek <lersek@redhat.com> wrote: > > Fix a recent regression (crash) in the multihead check; clean up the > code some more. > > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics) > Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics) > > Thanks, > Laszlo > > Laszlo Ersek (4): > ui/console: make qemu_console_is_multihead() static > ui/console: only walk QemuGraphicConsoles in > qemu_console_is_multihead() > ui/console: eliminate QOM properties from qemu_console_is_multihead() > ui/console: sanitize search in qemu_graphic_console_is_multihead() Series: Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> thanks > > include/ui/console.h | 1 - > ui/console.c | 24 +++++++++----------- > 2 files changed, 11 insertions(+), 14 deletions(-) >
On 9/15/23 13:50, Marc-André Lureau wrote: > Hi Laszlo > > On Wed, Sep 13, 2023 at 6:50 PM Laszlo Ersek <lersek@redhat.com> wrote: >> >> Fix a recent regression (crash) in the multihead check; clean up the >> code some more. >> >> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics) >> Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics) >> >> Thanks, >> Laszlo >> >> Laszlo Ersek (4): >> ui/console: make qemu_console_is_multihead() static >> ui/console: only walk QemuGraphicConsoles in >> qemu_console_is_multihead() >> ui/console: eliminate QOM properties from qemu_console_is_multihead() >> ui/console: sanitize search in qemu_graphic_console_is_multihead() > > Series: > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Thanks. Has this been queued by someone? Both Gerd and Marc-André are "odd fixers", so I'm not sure who should be sending a PR with these patches (and I don't see a pending PULL at <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> with these patch subjects included). Thanks. Laszlo
Hi Laszlo On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: > Has this been queued by someone? Both Gerd and Marc-André are "odd > fixers", so I'm not sure who should be sending a PR with these patches > (and I don't see a pending PULL at > <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> > with these patch subjects included). I have the series in my "ui" branch. I was waiting for a few more patches to be accumulated. But if someone else takes this first, I'll drop them.
On 26/09/2023 09:00, Marc-André Lureau wrote: > Hi Laszlo > > On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: >> Has this been queued by someone? Both Gerd and Marc-André are "odd >> fixers", so I'm not sure who should be sending a PR with these patches >> (and I don't see a pending PULL at >> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> >> with these patch subjects included). > > I have the series in my "ui" branch. I was waiting for a few more > patches to be accumulated. But if someone else takes this first, I'll > drop them. Does this series fix the "../ui/console.c:818: dpy_get_ui_info: Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when using gtk? It would be good to get this fixed in git master soon, as it has been broken for a couple of weeks now, and -display sdl has issues tracking the mouse correctly on my laptop here :( ATB, Mark.
On 9/29/23 09:57, Mark Cave-Ayland wrote: > On 26/09/2023 09:00, Marc-André Lureau wrote: > >> Hi Laszlo >> >> On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: >>> Has this been queued by someone? Both Gerd and Marc-André are "odd >>> fixers", so I'm not sure who should be sending a PR with these patches >>> (and I don't see a pending PULL at >>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> >>> with these patch subjects included). >> >> I have the series in my "ui" branch. I was waiting for a few more >> patches to be accumulated. But if someone else takes this first, I'll >> drop them. > > Does this series fix the "../ui/console.c:818: dpy_get_ui_info: > Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when > using gtk? It would be good to get this fixed in git master soon, as it > has been broken for a couple of weeks now, and -display sdl has issues > tracking the mouse correctly on my laptop here :( ... probably not; I've never seen that issue. Can you provide a reproducer? Also, it should be bisectable (over Marc-André's 52-part series I guess). Laszlo > > > ATB, > > Mark. >
On 30/09/2023 22:28, Laszlo Ersek wrote: > On 9/29/23 09:57, Mark Cave-Ayland wrote: >> On 26/09/2023 09:00, Marc-André Lureau wrote: >> >>> Hi Laszlo >>> >>> On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: >>>> Has this been queued by someone? Both Gerd and Marc-André are "odd >>>> fixers", so I'm not sure who should be sending a PR with these patches >>>> (and I don't see a pending PULL at >>>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> >>>> with these patch subjects included). >>> >>> I have the series in my "ui" branch. I was waiting for a few more >>> patches to be accumulated. But if someone else takes this first, I'll >>> drop them. >> >> Does this series fix the "../ui/console.c:818: dpy_get_ui_info: >> Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when >> using gtk? It would be good to get this fixed in git master soon, as it >> has been broken for a couple of weeks now, and -display sdl has issues >> tracking the mouse correctly on my laptop here :( > > ... probably not; I've never seen that issue. Can you provide a reproducer? The environment is a standard Debian bookworm install building QEMU git master with QEMU gtk support. The only difference I can think of is that I do all my QEMU builds as a separate user, and then export the display to my current user desktop i.e. As my current user: $ xhost + As my QEMU build user: $ export DISPLAY=:1 $ ./build/qemu-system-sparc qemu-system-sparc: ../ui/console.c:818: dpy_get_ui_info: Assertion `dpy_ui_info_supported(con)' failed. Aborted (core dumped) > Also, it should be bisectable (over Marc-André's 52-part series I guess). Indeed. I've just run git bisect and it returns the following: a92e7bb4cad57cc5c8817fb18fb25650507b69f8 is the first bad commit commit a92e7bb4cad57cc5c8817fb18fb25650507b69f8 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Tue Sep 12 10:13:01 2023 +0400 ui: add precondition for dpy_get_ui_info() Ensure that it only get called when dpy_ui_info_supported(). The function should always return a result. There should be a non-null console or active_console. Modify the argument to be const as well. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Albert Esteve <aesteve@redhat.com> include/ui/console.h | 2 +- ui/console.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) ATB, Mark.
On 10/1/23 08:15, Mark Cave-Ayland wrote: > On 30/09/2023 22:28, Laszlo Ersek wrote: > >> On 9/29/23 09:57, Mark Cave-Ayland wrote: >>> On 26/09/2023 09:00, Marc-André Lureau wrote: >>> >>>> Hi Laszlo >>>> >>>> On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: >>>>> Has this been queued by someone? Both Gerd and Marc-André are "odd >>>>> fixers", so I'm not sure who should be sending a PR with these patches >>>>> (and I don't see a pending PULL at >>>>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> >>>>> with these patch subjects included). >>>> >>>> I have the series in my "ui" branch. I was waiting for a few more >>>> patches to be accumulated. But if someone else takes this first, I'll >>>> drop them. >>> >>> Does this series fix the "../ui/console.c:818: dpy_get_ui_info: >>> Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when >>> using gtk? It would be good to get this fixed in git master soon, as it >>> has been broken for a couple of weeks now, and -display sdl has issues >>> tracking the mouse correctly on my laptop here :( >> >> ... probably not; I've never seen that issue. Can you provide a >> reproducer? > > The environment is a standard Debian bookworm install building QEMU git > master with QEMU gtk support. The only difference I can think of is that > I do all my QEMU builds as a separate user, and then export the display > to my current user desktop i.e. > > As my current user: > $ xhost + > > As my QEMU build user: > $ export DISPLAY=:1 > $ ./build/qemu-system-sparc > qemu-system-sparc: ../ui/console.c:818: dpy_get_ui_info: Assertion > `dpy_ui_info_supported(con)' failed. > Aborted (core dumped) > >> Also, it should be bisectable (over Marc-André's 52-part series I guess). > > Indeed. I've just run git bisect and it returns the following: > > a92e7bb4cad57cc5c8817fb18fb25650507b69f8 is the first bad commit > commit a92e7bb4cad57cc5c8817fb18fb25650507b69f8 > Author: Marc-André Lureau <marcandre.lureau@redhat.com> > Date: Tue Sep 12 10:13:01 2023 +0400 > > ui: add precondition for dpy_get_ui_info() > > Ensure that it only get called when dpy_ui_info_supported(). The > function should always return a result. There should be a non-null > console or active_console. > > Modify the argument to be const as well. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Albert Esteve <aesteve@redhat.com> > > include/ui/console.h | 2 +- > ui/console.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) This commit looks plain wrong to me; or rather I don't understand the argument. In the particular crash, we fail in gtk_display_init -> gtk_widget_show -> ... -> gd_configure -> gd_set_ui_size -> dpy_get_ui_info, and when the latter calls dpy_ui_info_supported(), we find that "con->hw_ops->ui_info" is NULL. In this particular case, "con->hw_ops" is "vga_ops", and indeed "vga_ops" does not provide an "ui_info" funcptr. SDL is unaffected because with SDL, we never call dpy_get_ui_info(). There's something fishy in the GTK display code BTW, in my opinion. I can't quite put my finger on it, but commit aeffd071ed81 ("ui: Deliver refresh rate via QemuUIInfo", 2022-06-14) definitely plays a role. Before commit aeffd071ed81, "ui/gtk.c" wouldn't call dpy_get_ui_info() either! Instead, from gd_configure(), we'd call gd_set_ui_info(), directly setting the size from the incoming GdkEventConfigure object. In commit aeffd071ed81, solely for the sake of carrying over the refresh rate, gd_set_ui_info() was renamed to gd_set_ui_size(). The width and height coming from the GdkEventConfigure object would be propagated the same way to dpy_set_ui_info(), but the *rest* of the QemuUIInfo object would be initialized differently. Before, the other fields would be zero, now they'd come from dpy_get_ui_info() -- most likely for the sake of carrying over the new refresh_rate field. This in itself wouldn't crash, but it set up the call chain that is now affected by the (IMO too strict) assertion. Why is a hw_ops-based ui_info needed for dpy_get_ui_info()? dpy_get_ui_info() never tries to *call* that function, it just returns &con->ui_info. So dpy_get_ui_info() *already* guarantees that it returns non-NULL. Laszlo > > > ATB, > > Mark. >
01.10.2023 12:31, Laszlo Ersek пишет: > On 10/1/23 08:15, Mark Cave-Ayland wrote: >> On 30/09/2023 22:28, Laszlo Ersek wrote: >> >>> On 9/29/23 09:57, Mark Cave-Ayland wrote: >>>> On 26/09/2023 09:00, Marc-André Lureau wrote: >>>> >>>>> Hi Laszlo >>>>> >>>>> On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> Has this been queued by someone? Both Gerd and Marc-André are "odd >>>>>> fixers", so I'm not sure who should be sending a PR with these patches >>>>>> (and I don't see a pending PULL at >>>>>> <https://lists.gnu.org/archive/html/qemu-devel/2023-09/threads.html> >>>>>> with these patch subjects included). >>>>> >>>>> I have the series in my "ui" branch. I was waiting for a few more >>>>> patches to be accumulated. But if someone else takes this first, I'll >>>>> drop them. >>>> >>>> Does this series fix the "../ui/console.c:818: dpy_get_ui_info: >>>> Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when >>>> using gtk? It would be good to get this fixed in git master soon, as it >>>> has been broken for a couple of weeks now, and -display sdl has issues >>>> tracking the mouse correctly on my laptop here :( >>> >>> ... probably not; I've never seen that issue. Can you provide a >>> reproducer? >> >> The environment is a standard Debian bookworm install building QEMU git >> master with QEMU gtk support. The only difference I can think of is that >> I do all my QEMU builds as a separate user, and then export the display >> to my current user desktop i.e. >> >> As my current user: >> $ xhost + >> >> As my QEMU build user: >> $ export DISPLAY=:1 >> $ ./build/qemu-system-sparc >> qemu-system-sparc: ../ui/console.c:818: dpy_get_ui_info: Assertion >> `dpy_ui_info_supported(con)' failed. >> Aborted (core dumped) >> >>> Also, it should be bisectable (over Marc-André's 52-part series I guess). >> >> Indeed. I've just run git bisect and it returns the following: >> >> a92e7bb4cad57cc5c8817fb18fb25650507b69f8 is the first bad commit >> commit a92e7bb4cad57cc5c8817fb18fb25650507b69f8 >> Author: Marc-André Lureau <marcandre.lureau@redhat.com> >> Date: Tue Sep 12 10:13:01 2023 +0400 >> >> ui: add precondition for dpy_get_ui_info() >> >> Ensure that it only get called when dpy_ui_info_supported(). The >> function should always return a result. There should be a non-null >> console or active_console. >> >> Modify the argument to be const as well. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Albert Esteve <aesteve@redhat.com> >> >> include/ui/console.h | 2 +- >> ui/console.c | 4 +++- >> 2 files changed, 4 insertions(+), 2 deletions(-) This is the first time the assertion failure has been found (right after merge of this series, Sep-13): https://lore.kernel.org/qemu-devel/801ac36e-b572-665b-5148-81baabf78a20@tls.msk.ru/ This is more discussion about it: https://lore.kernel.org/qemu-devel/0cae6d58-1476-9b92-0b48-f593b8e92ef2@tls.msk.ru/ > This commit looks plain wrong to me; or rather I don't understand the > argument. The thing here is that different parts of the code uses different object thinking it is the same thing. So it is confusing at best and smells wrong. But it is how it's been for quite some time, this new assert() just revealed the issue, I think. /mjt > In the particular crash, we fail in gtk_display_init -> gtk_widget_show > -> ... -> gd_configure -> gd_set_ui_size -> dpy_get_ui_info, and when > the latter calls dpy_ui_info_supported(), we find that > "con->hw_ops->ui_info" is NULL. In this particular case, "con->hw_ops" > is "vga_ops", and indeed "vga_ops" does not provide an "ui_info" funcptr. > > SDL is unaffected because with SDL, we never call dpy_get_ui_info(). > > There's something fishy in the GTK display code BTW, in my opinion. I > can't quite put my finger on it, but commit aeffd071ed81 ("ui: Deliver > refresh rate via QemuUIInfo", 2022-06-14) definitely plays a role. > > Before commit aeffd071ed81, "ui/gtk.c" wouldn't call dpy_get_ui_info() > either! Instead, from gd_configure(), we'd call gd_set_ui_info(), > directly setting the size from the incoming GdkEventConfigure object. > > In commit aeffd071ed81, solely for the sake of carrying over the refresh > rate, gd_set_ui_info() was renamed to gd_set_ui_size(). The width and > height coming from the GdkEventConfigure object would be propagated the > same way to dpy_set_ui_info(), but the *rest* of the QemuUIInfo object > would be initialized differently. Before, the other fields would be > zero, now they'd come from dpy_get_ui_info() -- most likely for the sake > of carrying over the new refresh_rate field. > > This in itself wouldn't crash, but it set up the call chain that is now > affected by the (IMO too strict) assertion. > > Why is a hw_ops-based ui_info needed for dpy_get_ui_info()? > dpy_get_ui_info() never tries to *call* that function, it just returns > &con->ui_info. So dpy_get_ui_info() *already* guarantees that it returns > non-NULL. > > Laszlo > >> >> >> ATB, >> >> Mark. >> > >
Fix a recent regression (crash) in the multihead check; clean up the code some more. Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics) Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics) Thanks, Laszlo Laszlo Ersek (4): ui/console: make qemu_console_is_multihead() static ui/console: only walk QemuGraphicConsoles in qemu_console_is_multihead() ui/console: eliminate QOM properties from qemu_console_is_multihead() ui/console: sanitize search in qemu_graphic_console_is_multihead() include/ui/console.h | 1 - ui/console.c | 24 +++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-)