[Qemu-devel] [PATCH 0/2] Modularize GTK Display

[Qemu-devel] [PATCH 0/2] Modularize GTK Display

[Colin Lord]

This series modularizes the GTK display. The first patch is from Fam's
series, which is necessary so that the module's libraries don't get
linked to the main binary. The second patch is responsible for
modularizing GTK.

This series is based off of my other series 'Dynamic module loading for
block drivers'.

Colin Lord (1):
  gtk: Modularize GTK display

Fam Zheng (1):
  rules.mak: Don't extract libs from .mo-libs in link command

 Makefile.objs        |  1 +
 configure            |  5 ++--
 include/ui/console.h |  6 +++++
 rules.mak            |  2 +-
 ui/Makefile.objs     | 14 ++++++++----
 ui/gtk-init.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui/gtk.c             | 11 +++++++--
 vl.c                 | 14 ++++++++++--
 8 files changed, 105 insertions(+), 12 deletions(-)
 create mode 100644 ui/gtk-init.c

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] rules.mak: Don't extract libs from .mo-libs in link command

[Colin Lord]

From: Fam Zheng <famz@redhat.com>

For module build, .mo objects are passed to LINK and consumed in
process-archive-undefs. The reason behind that is documented in the
comment above process-archive-undefs.

Similarly, extract-libs should be called with .mo filtered out too.
Otherwise, the .mo-libs are added to the link command incorrectly,
spoiling the purpose of modularization.

Currently we don't have any .mo-libs usage, but it will be used soon
when we modularize more multi-source objects, like sdl and gtk.

Reported-by: Colin Lord <clord@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 rules.mak | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rules.mak b/rules.mak
index 99cd0b3..1211649 100644
--- a/rules.mak
+++ b/rules.mak
@@ -50,7 +50,7 @@ process-archive-undefs = $(filter-out %.a %.mo,$1) \
                               $(call undefined-symbols,$(filter %.mo,$1)))) \
                 $(filter %.a,$1)
 
-extract-libs = $(strip $(foreach o,$1,$($o-libs)))
+extract-libs = $(strip $(foreach o,$(filter-out %.mo,$1),$($o-libs)))
 expand-objs = $(strip $(sort $(filter %.o,$1)) \
                   $(foreach o,$(filter %.mo,$1),$($o-objs)) \
                   $(filter-out %.o %.mo,$1))
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] gtk: Modularize GTK display

[Colin Lord]

Allows GTK to be compiled as a module and loaded at runtime. If GTK is
not the display being used, the module will not be loaded. In the case
that the GTK module is not present, if GTK was specified explicitly by
the user, qemu will exit with an error, otherwise it will default to
display type None.

This should give improvements in terms of performance, since much less
libraries will be loaded at startup if GTK is not being used, and in
terms of packaging, since GTK should be able to be packaged separately
in binary package managers now.

Signed-off-by: Colin Lord <clord@redhat.com>
---
 Makefile.objs        |  1 +
 configure            |  5 ++--
 include/ui/console.h |  6 +++++
 ui/Makefile.objs     | 14 ++++++++----
 ui/gtk-init.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui/gtk.c             | 11 +++++++--
 vl.c                 | 14 ++++++++++--
 7 files changed, 104 insertions(+), 11 deletions(-)
 create mode 100644 ui/gtk-init.c

diff --git a/Makefile.objs b/Makefile.objs
index 6d5ddcf..08c5746 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -62,6 +62,7 @@ common-obj-y += accel.o
 common-obj-y += replay/
 
 common-obj-y += ui/
+common-obj-m += ui/
 common-obj-y += bt-host.o bt-vhci.o
 bt-host.o-cflags := $(BLUEZ_CFLAGS)
 
diff --git a/configure b/configure
index f1e7d14..ab15662 100755
--- a/configure
+++ b/configure
@@ -2185,7 +2185,6 @@ if test "$gtk" != "no"; then
             gtk_cflags="$gtk_cflags $x11_cflags"
             gtk_libs="$gtk_libs $x11_libs"
         fi
-        libs_softmmu="$gtk_libs $libs_softmmu"
         gtk="yes"
     elif test "$gtk" = "yes"; then
         feature_not_found "gtk" "Install gtk2 or gtk3 devel"
@@ -2422,7 +2421,6 @@ if test "$vte" != "no"; then
         vte_cflags=$($pkg_config --cflags $vtepackage)
         vte_libs=$($pkg_config --libs $vtepackage)
         vteversion=$($pkg_config --modversion $vtepackage)
-        libs_softmmu="$vte_libs $libs_softmmu"
         vte="yes"
     elif test "$vte" = "yes"; then
         if test "$gtkabi" = "3.0"; then
@@ -5166,7 +5164,7 @@ if test "$glib_subprocess" = "yes" ; then
 fi
 echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
 if test "$gtk" = "yes" ; then
-  echo "CONFIG_GTK=y" >> $config_host_mak
+  echo "CONFIG_GTK=m" >> $config_host_mak
   echo "CONFIG_GTKABI=$gtkabi" >> $config_host_mak
   echo "GTK_CFLAGS=$gtk_cflags" >> $config_host_mak
   echo "GTK_LIBS=$gtk_libs" >> $config_host_mak
@@ -5211,6 +5209,7 @@ fi
 if test "$vte" = "yes" ; then
   echo "CONFIG_VTE=y" >> $config_host_mak
   echo "VTE_CFLAGS=$vte_cflags" >> $config_host_mak
+  echo "VTE_LIBS=$vte_libs" >> $config_host_mak
 fi
 if test "$virglrenderer" = "yes" ; then
   echo "CONFIG_VIRGL=y" >> $config_host_mak
diff --git a/include/ui/console.h b/include/ui/console.h
index 2703a3a..45f11e0 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -502,8 +502,14 @@ int index_from_key(const char *key, size_t key_length);
 
 /* gtk.c */
 #ifdef CONFIG_GTK
+bool gtk_mod_init(void);
 void early_gtk_display_init(int opengl);
 void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover);
+void early_gtk_display_init_do(int opengl);
+void gtk_display_init_do(DisplayState *ds, bool full_screen,
+                         bool grab_on_hover);
+void gtk_register_early_init_fun(void *fn);
+void gtk_register_init_fun(void *fn);
 #else
 static inline void gtk_display_init(DisplayState *ds, bool full_screen,
                                     bool grab_on_hover)
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index dc936f1..0e88c46 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -15,7 +15,11 @@ common-obj-$(CONFIG_SDL) += sdl.mo x_keymap.o
 common-obj-$(CONFIG_COCOA) += cocoa.o
 common-obj-$(CONFIG_CURSES) += curses.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
-common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
+common-obj-$(CONFIG_GTK) += gtk.mo
+
+ifneq ($(CONFIG_GTK),)
+common-obj-y += gtk-init.o x_keymap.o
+endif
 
 ifeq ($(CONFIG_SDLABI),1.2)
 sdl.mo-objs := sdl.o sdl_zoom.o
@@ -28,25 +32,27 @@ endif
 endif
 sdl.mo-cflags := $(SDL_CFLAGS)
 
+gtk.mo-objs := gtk.o
 ifeq ($(CONFIG_OPENGL),y)
 common-obj-y += shader.o
 common-obj-y += console-gl.o
 common-obj-y += egl-helpers.o
 common-obj-y += egl-context.o
 ifeq ($(CONFIG_GTK_GL),y)
-common-obj-$(CONFIG_GTK) += gtk-gl-area.o
+gtk.mo-objs += gtk-gl-area.o
 else
-common-obj-$(CONFIG_GTK) += gtk-egl.o
+gtk.mo-objs += gtk-egl.o
 endif
 endif
 
-gtk.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS)
+gtk.mo-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS)
 gtk-egl.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) $(OPENGL_CFLAGS)
 gtk-gl-area.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) $(OPENGL_CFLAGS)
 shader.o-cflags += $(OPENGL_CFLAGS)
 console-gl.o-cflags += $(OPENGL_CFLAGS)
 egl-helpers.o-cflags += $(OPENGL_CFLAGS)
 
+gtk.mo-libs := $(GTK_LIBS) $(VTE_LIBS)
 gtk-egl.o-libs += $(OPENGL_LIBS)
 shader.o-libs += $(OPENGL_LIBS)
 console-gl.o-libs += $(OPENGL_LIBS)
diff --git a/ui/gtk-init.c b/ui/gtk-init.c
new file mode 100644
index 0000000..697972f
--- /dev/null
+++ b/ui/gtk-init.c
@@ -0,0 +1,64 @@
+/*
+ * QEMU GTK display driver init function
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2016 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "ui/console.h"
+#include "qemu/module.h"
+
+static void (*early_init_fn)(int opengl);
+static void (*init_fn)(DisplayState *ds, bool full_screen, bool grab_on_hover);
+
+void gtk_register_early_init_fun(void *fn)
+{
+    assert(!early_init_fn);
+    early_init_fn = fn;
+}
+
+void gtk_register_init_fun(void *fn)
+{
+    assert(!init_fn);
+    init_fn = fn;
+}
+
+bool gtk_mod_init(void)
+{
+    module_load_one("ui-", "gtk");
+    if (!early_init_fn || !init_fn) {
+        return false;
+    }
+    return true;
+}
+
+void early_gtk_display_init(int opengl)
+{
+    assert(early_init_fn);
+    early_init_fn(opengl);
+}
+
+void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover)
+{
+    assert(init_fn);
+    init_fn(ds, full_screen, grab_on_hover);
+}
diff --git a/ui/gtk.c b/ui/gtk.c
index 58d20ee..a698144 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2150,7 +2150,7 @@ static void gd_set_keycode_type(GtkDisplayState *s)
 
 static gboolean gtkinit;
 
-void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover)
+void gtk_display_init_do(DisplayState *ds, bool full_screen, bool grab_on_hover)
 {
     GtkDisplayState *s = g_malloc0(sizeof(*s));
     char *filename;
@@ -2240,7 +2240,7 @@ void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover)
     gd_set_keycode_type(s);
 }
 
-void early_gtk_display_init(int opengl)
+void early_gtk_display_init_do(int opengl)
 {
     /* The QEMU code relies on the assumption that it's always run in
      * the C locale. Therefore it is not prepared to deal with
@@ -2288,3 +2288,10 @@ void early_gtk_display_init(int opengl)
     register_vc_handler(gd_vc_handler);
 #endif
 }
+
+__attribute__((constructor))
+static void gtk_init_fn(void)
+{
+    gtk_register_early_init_fun(early_gtk_display_init_do);
+    gtk_register_init_fun(gtk_display_init_do);
+}
diff --git a/vl.c b/vl.c
index 1a5f807..5705a0a 100644
--- a/vl.c
+++ b/vl.c
@@ -4232,9 +4232,19 @@ int main(int argc, char **argv, char **envp)
         display_remote++;
     }
 #endif
-    if (display_type == DT_DEFAULT && !display_remote) {
+    if ((display_type == DT_DEFAULT && !display_remote)
+        || display_type == DT_GTK) {
 #if defined(CONFIG_GTK)
-        display_type = DT_GTK;
+        if (!gtk_mod_init()) {
+            if (display_type == DT_GTK) {
+                error_report("GTK could not be initialized, exiting");
+                exit(1);
+            }
+            error_report("GTK could not be initialized, using display None");
+            display_type = DT_NONE;
+        } else {
+            display_type = DT_GTK;
+        }
 #elif defined(CONFIG_SDL)
         display_type = DT_SDL;
 #elif defined(CONFIG_COCOA)
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/2] gtk: Modularize GTK display

[Gerd Hoffmann]

  Hi,

> +static void (*early_init_fn)(int opengl);
> +static void (*init_fn)(DisplayState *ds, bool full_screen, bool grab_on_hover);
> +
> +void gtk_register_early_init_fun(void *fn)
> +{
> +    assert(!early_init_fn);
> +    early_init_fn = fn;
> +}
> +
> +void gtk_register_init_fun(void *fn)
> +{
> +    assert(!init_fn);
> +    init_fn = fn;
> +}

Hmm, do we _really_ want a ad-hoc interface like this, different for
each and every UI?

I think it would be a good idea cleanup the display init code first,
then go modularize the UIs.  Switch the option parsing to QemuOpts (vnc
and spice already use them).  With that in place the UI init functions
can have identical prototypes (like vnc_init_func).

Which in turn allows to have something like ...

struct ui_driver {
    const char *name;
    int type                           /* DT_FOO */

    QemuOptsList *opts;
    bool is_remote;                    /* vnc, spice */
    bool supports_multiple_instances;  /* vnc */
    bool supports_opengl;              /* sdl2, gtk, spice */

    void (*early_init)(bool initialize_opengl, Error *errp);
    void (*init)(void *opaque, QemuOpts *opts, Error **errp);
}

... and register *that*.

We'll also need to sort how to handle monitor integration best (set
password for spice+vnc, disable/enable/reconfigure vnc, "info
spice", ...).  When starting with the local displays (gtk, sdl) we can
defer that for now though.

Another question is what we want do with sdl1 support.  With UIs built
as module we might be able to allow picking SDL1 or SDL2 at runtime not
compile time.  Need to take care we never try to load both though.  And
maybe we should simply ditch SDL1 support and focus on SDL2 instead.

> diff --git a/vl.c b/vl.c
> index 1a5f807..5705a0a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4232,9 +4232,19 @@ int main(int argc, char **argv, char **envp)
>          display_remote++;
>      }
>  #endif
> -    if (display_type == DT_DEFAULT && !display_remote) {
> +    if ((display_type == DT_DEFAULT && !display_remote)
> +        || display_type == DT_GTK) {
>  #if defined(CONFIG_GTK)
> -        display_type = DT_GTK;
> +        if (!gtk_mod_init()) {
> +            if (display_type == DT_GTK) {
> +                error_report("GTK could not be initialized, exiting");
> +                exit(1);
> +            }
> +            error_report("GTK could not be initialized, using display None");
> +            display_type = DT_NONE;
> +        } else {
> +            display_type = DT_GTK;
> +        }
>  #elif defined(CONFIG_SDL)
>          display_type = DT_SDL;
>  #elif defined(CONFIG_COCOA)

I think you don't load the gtk module on "qemu -display gtk".

Also with modularizing the UIs the decision which UI to pick as default
effectively moves from compile time to runtime and we need something
more clever than that.  IMHO qemu compiled without gtk and qemu compiled
with gtk but gtk module not installed should behave identical, i.e.
fallback to sdl or vnc instead of using DT_NONE.

cheers,
  Gerd