Re: [PATCH 0/4] ui/console: multihead: fix crash, simplify logic

[Laszlo Ersek <lersek@redhat.com>]

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.
>