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

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

[Laszlo Ersek]

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(-)

[PATCH 1/4] ui/console: make qemu_console_is_multihead() static

[Laszlo Ersek]

qemu_console_is_multihead() is only called from within "ui/console.c";
make it static.

Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics)
Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/ui/console.h | 1 -
 ui/console.c         | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 1ccd432b4d64..d715f88b1be2 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -506,7 +506,6 @@ bool qemu_console_is_visible(QemuConsole *con);
 bool qemu_console_is_graphic(QemuConsole *con);
 bool qemu_console_is_fixedsize(QemuConsole *con);
 bool qemu_console_is_gl_blocked(QemuConsole *con);
-bool qemu_console_is_multihead(DeviceState *dev);
 char *qemu_console_get_label(QemuConsole *con);
 int qemu_console_get_index(QemuConsole *con);
 uint32_t qemu_console_get_head(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index e4d61794bb2c..adacc3473140 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2365,7 +2365,7 @@ bool qemu_console_is_gl_blocked(QemuConsole *con)
     return con->gl_block;
 }
 
-bool qemu_console_is_multihead(DeviceState *dev)
+static bool qemu_console_is_multihead(DeviceState *dev)
 {
     QemuConsole *con;
     Object *obj;

Re: [PATCH 1/4] ui/console: make qemu_console_is_multihead() static

[Philippe Mathieu-Daudé]

On 13/9/23 16:49, Laszlo Ersek wrote:
> qemu_console_is_multihead() is only called from within "ui/console.c";
> make it static.
> 
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics)
> Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   include/ui/console.h | 1 -
>   ui/console.c         | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/4] ui/console: only walk QemuGraphicConsoles in qemu_console_is_multihead()

[Laszlo Ersek]

qemu_console_is_multihead() declares the console "c" a "multihead" console
if there are two different consoles in the system that (a) both reference
"c->device", and (b) have different "c->head" numbers. In effect, if at
least two consoles exist that are different heads of the same device that
underlies "c".

Commit 58d5870845c6 ("ui/console: move graphic fields to
QemuGraphicConsole", 2023-09-04) pushed the "device" and "head" members
from the QemuConsole base class down to the QemuGraphicConsole subclass,
adjusting the referring QOM properties accordingly as well. As a result,
the "device" property lookup in qemu_console_is_multihead() now crashes,
in case the candidate console being investigated for criterion (a) is not
a QemuGraphicConsole instance:

> Unexpected error in object_property_find_err() at qom/object.c:1314:
> qemu: Property 'qemu-fixed-text-console.device' not found
> Aborted (core dumped)

This is effectively an unchecked downcast. Make it checked: only consider
such console candidates that are themselves QemuGraphicConsole instances.

Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics)
Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
Fixes: 58d5870845c6
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ui/console.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ui/console.c b/ui/console.c
index adacc3473140..2ee65207b430 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2373,6 +2373,9 @@ static bool qemu_console_is_multihead(DeviceState *dev)
     uint32_t h;
 
     QTAILQ_FOREACH(con, &consoles, next) {
+        if (!QEMU_IS_GRAPHIC_CONSOLE(con)) {
+            continue;
+        }
         obj = object_property_get_link(OBJECT(con),
                                        "device", &error_abort);
         if (DEVICE(obj) != dev) {

[PATCH 3/4] ui/console: eliminate QOM properties from qemu_console_is_multihead()

[Laszlo Ersek]

According to Marc-André's and Gerd's descriptions, the "device" and
"head" members of QemuGraphicConsole are exposed as QOM properties for two
purposes:

(1) Introspection (e.g., "qom-get" monitor command).

(2) A VNC server can display a specific device + head. This lets us run a
    multihead configuration by using multiple VNC servers (one for each
    head).

    Further, we can link input devices to device + head, so input events
    are routed to different devices dependent on where they are coming
    from. Which is most useful for tablet devices in a VNC multihead
    setup, each head has its own tablet device then. This does requires
    manual guest-side configuration, for establishing the same tablet <->
    head relationship.

However, neither goal seems to justify the complicated QOM property lookup
that's internal to qemu_console_is_multihead().

Rework qemu_console_is_multihead() with plain old C language field
accesses.

Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics)
Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ui/console.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 2ee65207b430..6424820c8521 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2365,25 +2365,25 @@ bool qemu_console_is_gl_blocked(QemuConsole *con)
     return con->gl_block;
 }
 
-static bool qemu_console_is_multihead(DeviceState *dev)
+static bool qemu_graphic_console_is_multihead(QemuGraphicConsole *c)
 {
     QemuConsole *con;
-    Object *obj;
     uint32_t f = 0xffffffff;
     uint32_t h;
 
     QTAILQ_FOREACH(con, &consoles, next) {
+        QemuGraphicConsole *candidate;
+
         if (!QEMU_IS_GRAPHIC_CONSOLE(con)) {
             continue;
         }
-        obj = object_property_get_link(OBJECT(con),
-                                       "device", &error_abort);
-        if (DEVICE(obj) != dev) {
+
+        candidate = QEMU_GRAPHIC_CONSOLE(con);
+        if (candidate->device != c->device) {
             continue;
         }
 
-        h = object_property_get_uint(OBJECT(con),
-                                     "head", &error_abort);
+        h = candidate->head;
         if (f == 0xffffffff) {
             f = h;
         } else if (h != f) {
@@ -2402,7 +2402,7 @@ char *qemu_console_get_label(QemuConsole *con)
             bool multihead;
 
             dev = DEVICE(c->device);
-            multihead = qemu_console_is_multihead(dev);
+            multihead = qemu_graphic_console_is_multihead(c);
             if (multihead) {
                 return g_strdup_printf("%s.%d", dev->id ?
                                        dev->id :

[PATCH 4/4] ui/console: sanitize search in qemu_graphic_console_is_multihead()

[Laszlo Ersek]

qemu_graphic_console_is_multihead() declares the graphical console "c" a
"multihead" console if there are two different graphical consoles in the
system that (a) both reference "c->device", and (b) have different
"c->head" numbers. In effect, if at least two graphical consoles exist
that are different heads of the same device that underlies "c". In fact,
"c" may be one of these two graphical consoles, or "c" may differ from
both of those consoles (in case "c->device" has at least three heads).

The loop currently uses this awkward "two different consoles" approach
because the function used not to have access to "c", only to "c->device",
which didn't allow for fetching (and comparing) "c->head". But, we've
changed that in the last patch; we now pass all of "c" to
qemu_graphic_console_is_multihead().

Thus, look for the *first* (and possibly *only*) graphical console, if
any, that refers to the same "device" as "c", but by a different "head"
number.

Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com> (odd fixer:Graphics)
Cc: Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    context:-U4

 ui/console.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 6424820c8521..9ce3c1248c7c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2367,10 +2367,8 @@ bool qemu_console_is_gl_blocked(QemuConsole *con)
 
 static bool qemu_graphic_console_is_multihead(QemuGraphicConsole *c)
 {
     QemuConsole *con;
-    uint32_t f = 0xffffffff;
-    uint32_t h;
 
     QTAILQ_FOREACH(con, &consoles, next) {
         QemuGraphicConsole *candidate;
 
@@ -2382,12 +2380,9 @@ static bool qemu_graphic_console_is_multihead(QemuGraphicConsole *c)
         if (candidate->device != c->device) {
             continue;
         }
 
-        h = candidate->head;
-        if (f == 0xffffffff) {
-            f = h;
-        } else if (h != f) {
+        if (candidate->head != c->head) {
             return true;
         }
     }
     return false;

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

[Marc-André Lureau]

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

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

[Laszlo Ersek]

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

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

[Marc-André Lureau]

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

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

[Mark Cave-Ayland]

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.

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

[Laszlo Ersek]

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

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

[Mark Cave-Ayland]

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.

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

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

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

[Michael Tokarev]

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