Re: [Qemu-devel] [PATCH for 2.8 3/3] sdl: Modularize

[Paolo Bonzini]

On 27/07/2016 08:26, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  Makefile.objs         |  1 +
>  configure             |  4 +--
>  include/qemu/module.h |  2 ++
>  include/ui/console.h  |  5 ++--
>  ui/Makefile.objs      |  2 ++
>  ui/sdl-init.c         | 75 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  ui/sdl.c              | 19 ++++++-------
>  ui/sdl2.c             | 26 ++++++------------
>  util/module.c         |  6 +++++
>  vl.c                  |  5 +++-
>  10 files changed, 111 insertions(+), 34 deletions(-)
>  create mode 100644 ui/sdl-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 e04e59f..1b9be8f 100755
> --- a/configure
> +++ b/configure
> @@ -2533,7 +2533,6 @@ EOF
>      sdl_cflags="$sdl_cflags $x11_cflags"
>      sdl_libs="$sdl_libs $x11_libs"
>    fi
> -  libs_softmmu="$sdl_libs $libs_softmmu"
>  fi
>  
>  ##########################################
> @@ -5065,9 +5064,10 @@ if test "$modules" = "yes"; then
>    echo "CONFIG_MODULES=y" >> $config_host_mak
>  fi
>  if test "$sdl" = "yes" ; then
> -  echo "CONFIG_SDL=y" >> $config_host_mak
> +  echo "CONFIG_SDL=m" >> $config_host_mak
>    echo "CONFIG_SDLABI=$sdlabi" >> $config_host_mak
>    echo "SDL_CFLAGS=$sdl_cflags" >> $config_host_mak
> +  echo "SDL_LIBS=$sdl_libs" >> $config_host_mak
>  fi
>  if test "$sdlabi" = "2.0"; then
>    echo "CONFIG_SDL2=y" >> $config_host_mak
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 2370708..f5e012b 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -44,6 +44,7 @@ typedef enum {
>      MODULE_INIT_OPTS,
>      MODULE_INIT_QAPI,
>      MODULE_INIT_QOM,
> +    MODULE_INIT_SDL,
>      MODULE_INIT_MAX
>  } module_init_type;
>  
> @@ -51,6 +52,7 @@ typedef enum {
>  #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
>  #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
>  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> +#define sdl_init(function) module_init(function, MODULE_INIT_SDL)
>  
>  void register_module_init(void (*fn)(void), module_init_type type);
>  void register_dso_module_init(void (*fn)(void), module_init_type type);
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 2703a3a..1d9b0bb 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -425,10 +425,11 @@ void surface_gl_setup_viewport(ConsoleGLState *gls,
>  
>  /* sdl.c */
>  #ifdef CONFIG_SDL
> -void sdl_display_early_init(int opengl);
> +bool sdl_display_early_init(int opengl);
>  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
> +void sdl_register_init_fun(void *fn);
>  #else
> -static inline void sdl_display_early_init(int opengl)
> +static inline bool sdl_display_early_init(int opengl)
>  {
>      /* This must never be called if CONFIG_SDL is disabled */
>      error_report("SDL support is disabled");
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index dc936f1..0b82650 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -26,7 +26,9 @@ ifeq ($(CONFIG_OPENGL),y)
>  sdl.mo-objs += sdl2-gl.o
>  endif
>  endif
> +common-obj-y += sdl-init.o
>  sdl.mo-cflags := $(SDL_CFLAGS)
> +sdl.mo-libs := $(SDL_LIBS)
>  
>  ifeq ($(CONFIG_OPENGL),y)
>  common-obj-y += shader.o
> diff --git a/ui/sdl-init.c b/ui/sdl-init.c
> new file mode 100644
> index 0000000..6a780a4
> --- /dev/null
> +++ b/ui/sdl-init.c
> @@ -0,0 +1,75 @@
> +/*
> + * QEMU SDL 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 (*init_fn)(DisplayState *ds, int full_screen, int no_frame);
> +void sdl_register_init_fun(void *fn)
> +{
> +    assert(!init_fn);
> +    init_fn = fn;
> +}
> +
> +bool sdl_display_early_init(int opengl)
> +{
> +
> +#ifdef CONFIG_SDL2
> +    switch (opengl) {
> +    case -1: /* default */
> +    case 0:  /* off */
> +        break;
> +    case 1: /* on */
> +#ifdef CONFIG_OPENGL
> +        display_opengl = 1;
> +#endif
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +#else
> +    if (opengl == 1 /* on */) {
> +        fprintf(stderr,
> +                "SDL1 display code has no opengl support.\n"
> +                "Please recompile qemu with SDL2, using\n"
> +                "./configure --enable-sdl --with-sdlabi=2.0\n");
> +        /* XXX: Should we return false here? */
> +    }
> +#endif
> +
> +    module_call_init(MODULE_INIT_SDL);
> +    if (!init_fn) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +{
> +    assert(init_fn);
> +    init_fn(ds, full_screen, no_frame);
> +}
> diff --git a/ui/sdl.c b/ui/sdl.c
> index d8cf5bc..a30f442 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -932,17 +932,7 @@ static const DisplayChangeListenerOps dcl_ops = {
>      .dpy_cursor_define    = sdl_mouse_define,
>  };
>  
> -void sdl_display_early_init(int opengl)
> -{
> -    if (opengl == 1 /* on */) {
> -        fprintf(stderr,
> -                "SDL1 display code has no opengl support.\n"
> -                "Please recompile qemu with SDL2, using\n"
> -                "./configure --enable-sdl --with-sdlabi=2.0\n");
> -    }
> -}
> -
> -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +static void sdl_display_init_do(DisplayState *ds, int full_screen, int 
> no_frame)
>  {
>      int flags;
>      uint8_t data = 0;
> @@ -1025,3 +1015,10 @@ void sdl_display_init(DisplayState *ds, int 
> full_screen, int no_frame)
>  
>      atexit(sdl_cleanup);
>  }
> +
> +static void sdl_init_fn(void)
> +{
> +    sdl_register_init_fun(sdl_display_init_do);
> +}
> +
> +sdl_init(sdl_init_fn);

Can you use __attribute__((constructor)) instead of going through the
burden of defining a new type?  The registration function only does a
single assignment, so it doesn't really have ordering dependencies.

Thanks,

Paolo

> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index 30d2a3c..c2b4049 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -738,24 +738,7 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
>  };
>  #endif
>  
> -void sdl_display_early_init(int opengl)
> -{
> -    switch (opengl) {
> -    case -1: /* default */
> -    case 0:  /* off */
> -        break;
> -    case 1: /* on */
> -#ifdef CONFIG_OPENGL
> -        display_opengl = 1;
> -#endif
> -        break;
> -    default:
> -        g_assert_not_reached();
> -        break;
> -    }
> -}
> -
> -void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
> +static void sdl_display_init_do(DisplayState *ds, int full_screen, int 
> no_frame)
>  {
>      int flags;
>      uint8_t data = 0;
> @@ -842,3 +825,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
> int no_frame)
>  
>      atexit(sdl_cleanup);
>  }
> +
> +static void sdl_init_fn(void)
> +{
> +    sdl_register_init_fun(sdl_display_init_do);
> +}
> +
> +sdl_init(sdl_init_fn);
> diff --git a/util/module.c b/util/module.c
> index 86e3f7a..b95d4fa 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -167,6 +167,9 @@ static void module_load(module_init_type type)
>      static const char *block_modules[] = {
>          CONFIG_BLOCK_MODULES
>      };
> +    static const char *sdl_modules[] = {
> +        "ui-sdl",
> +    };
>      char *exec_dir;
>      char *dirs[3];
>      int i = 0;
> @@ -181,6 +184,9 @@ static void module_load(module_init_type type)
>      case MODULE_INIT_BLOCK:
>          mp = block_modules;
>          break;
> +    case MODULE_INIT_SDL:
> +        mp = sdl_modules;
> +        break;
>      default:
>          /* no other types have dynamic modules for now*/
>          return;
> diff --git a/vl.c b/vl.c
> index a455947..fdbbe47 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4221,7 +4221,10 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (display_type == DT_SDL) {
> -        sdl_display_early_init(request_opengl);
> +        if (!sdl_display_early_init(request_opengl)) {
> +            error_report("Failed to initialize SDL");
> +            exit(1);
> +        }
>      }
>  
>      if (request_opengl == 1 && display_opengl == 0) {
>