Re: [PATCH 0/6] ui/dbus: Share one listener for a console

[Akihiko Odaki]

On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi Akihiko
>
> On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> ui/dbus required to have multiple DisplayChangeListeners (possibly with 
>> OpenGL)
>> for a console but that caused several problems:
>> - It broke egl-headless, an unusual display which implements OpenGL rendering
>>   for non-OpenGL displays. The code to support multiple 
>> DisplayChangeListeners
>>   does not consider the case where non-OpenGL displays listens OpenGL 
>> consoles.
>
>
> Can you provide instructions on what broke? Even better write a test, please.

The following command segfaults. Adding a test would be nice, but it
would need a binary which uses OpenGL.
qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless
-vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel
kvm

>
> "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py", which covers 
> egl-headless usage, works.
>
>>
>> - Multiple OpenGL DisplayChangeListeners of dbus shares one DisplaySurface 
>> and
>>   modifies its texture field, causing OpenGL texture leak and use-after-free.
>
>
> Again, please provide instructions. I have regularly run -display dbus with 
> multiple clients and qemu compiled with sanitizers. I don't see any leak or 
> use after free.

I doubt sanitizers can find this because it is an OpenGL texture. You
may add a probe around surface_gl_create_texture and
surface_gl_destroy_texture.

>
>>
>> - Introduced extra code to check the compatibility of OpenGL context 
>> providers
>>   and OpenGL texture renderers where those are often inherently tightly 
>> coupled
>>   since they must share the same hardware.
>
>
> So code checks are meant to prevent misusage. They might be too limited or 
> broken in some ways, but reverting is likely going to introduce other 
> regressions I was trying to fix.

The misuse will not occur because DisplayChangeListeners will be
merged with OpenGL context providers.

>
>> - Needed extra work to broadcast the same change to multiple dbus listeners.
>>
>
> Compared to what?

Compared to sharing one DisplayChangeListener for multiple dbus listeners.

>
>>
>> This series solve them by implementing the change broadcast in ui/dbus, which
>> knows exactly what is needed. Changes for the common code to support multiple
>> OpenGL DisplayChangeListeners were reverted to fix egl-headless and reduce
>> the code size.
>
>
> Thanks a lot for your work, I am going to take a look at your approach. But 
> please help us understand better what the problem actually is, by giving 
> examples & tests to avoid future regressions and document the expected 
> behaviour.

The thing is really complicated and I may miss details so please feel
free to ask questions or provide suggestions.

Regards,
Akihiko Odaki


>
>>
>> Akihiko Odaki (6):
>>   ui/dbus: Share one listener for a console
>>   Revert "console: save current scanout details"
>>   Revert "ui: split the GL context in a different object"
>>   Revert "ui: dispatch GL events to all listeners"
>>   Revert "ui: associate GL context outside of display listener
>>     registration"
>>   Revert "ui: factor out qemu_console_set_display_gl_ctx()"
>>
>>  include/ui/console.h       |  60 +-----
>>  include/ui/egl-context.h   |   6 +-
>>  include/ui/gtk.h           |  11 +-
>>  include/ui/sdl2.h          |   7 +-
>>  include/ui/spice-display.h |   1 -
>>  ui/console.c               | 258 +++++++----------------
>>  ui/dbus-console.c          | 109 ++--------
>>  ui/dbus-listener.c         | 417 +++++++++++++++++++++++++++----------
>>  ui/dbus.c                  |  22 --
>>  ui/dbus.h                  |  36 +++-
>>  ui/egl-context.c           |   6 +-
>>  ui/egl-headless.c          |  20 +-
>>  ui/gtk-egl.c               |  10 +-
>>  ui/gtk-gl-area.c           |   8 +-
>>  ui/gtk.c                   |  25 +--
>>  ui/sdl2-gl.c               |  10 +-
>>  ui/sdl2.c                  |  14 +-
>>  ui/spice-display.c         |  19 +-
>>  18 files changed, 498 insertions(+), 541 deletions(-)
>>
>> --
>> 2.32.0 (Apple Git-132)
>>
>>
>
>
> --
> Marc-André Lureau