include/ui/console.h | 1 - ui/console.c | 24 +++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-)
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(-)
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(-) > -- Marc-André Lureau
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. -- Marc-André Lureau
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.
>>
>
>
© 2016 - 2024 Red Hat, Inc.