Re: [PATCH weston 2/3] modules: Drop module_init as a shared init function

[Pekka Paalanen]

On Mon, 11 Jul 2016 11:05:38 +0200
Giulio Camuffo  wrote:

> 2016-07-04 15:58 GMT+02:00 Quentin Glidic :
> > From: Quentin Glidic 
> >
> > Use different functions so we cannot load a libweston module in weston
> > or the other way around.
> >
> > Also properly namespace backend_init and use a different name for weston
> > shells.
> >
> > Signed-off-by: Quentin Glidic 
> > ---
> >  compositor/cms-colord.c |  5 +++--
> >  compositor/cms-static.c |  4 ++--
> >  compositor/main.c   | 45 
> > ++---
> >  compositor/screen-share.c   |  4 ++--
> >  compositor/weston.h | 13 ++-
> >  desktop-shell/shell.c   |  4 ++--
> >  fullscreen-shell/fullscreen-shell.c |  5 +++--
> >  ivi-shell/ivi-layout.c  |  4 +++-
> >  ivi-shell/ivi-shell.c   |  4 ++--
> >  libweston/compositor-drm.c  |  4 ++--
> >  libweston/compositor-fbdev.c|  4 ++--
> >  libweston/compositor-headless.c |  4 ++--
> >  libweston/compositor-rdp.c  |  4 ++--
> >  libweston/compositor-wayland.c  |  4 ++--
> >  libweston/compositor-x11.c  |  4 ++--
> >  libweston/compositor.c  | 23 ---
> >  libweston/compositor.h  |  7 +++---
> >  tests/plugin-registry-test.c|  4 +++-
> >  tests/surface-global-test.c |  4 +++-
> >  tests/surface-screenshot.c  |  5 +++--
> >  tests/surface-test.c|  4 +++-
> >  tests/weston-test.c |  4 ++--
> >  xwayland/launcher.c |  3 +--
> >  23 files changed, 111 insertions(+), 55 deletions(-)

Hi, 

this patch could have been split at least between libweston and
compositor if not even per module type. I don't think there were any
interdependencies. Yes, this patch is quite straightforward, but
reading the commit message it lists three different things it does and
while the diffstat is fairly small, the number of affected files is
fairly large.

> > diff --git a/compositor/main.c b/compositor/main.c
> > index 4e6b7ae..88f7911 100644
> > --- a/compositor/main.c
> > +++ b/compositor/main.c
> > @@ -704,7 +704,7 @@ weston_create_listening_socket(struct wl_display 
> > *display, const char *socket_na
> >  }
> >
> >  WL_EXPORT void *
> > -wet_load_module(const char *name, const char *entrypoint)
> > +wet_load_module_entrypoint(const char *name, const char *entrypoint)
> >  {
> > const char *builddir = getenv("WESTON_BUILD_DIR");
> > char path[PATH_MAX];
> > @@ -746,14 +746,46 @@ wet_load_module(const char *name, const char 
> > *entrypoint)
> > return init;
> >  }
> >
> > +static int
> > +wet_load_shell(struct weston_compositor *compositor,
> > +  const char *name, int *argc, char *argv[])
> > +{
> > +   int (*shell_init)(struct weston_compositor *ec,
> > + int *argc, char *argv[]);
> > +
> > +   shell_init = wet_load_module_entrypoint(name, "wet_shell_init");
> > +   if (!shell_init)
> > +   shell_init = wet_load_module_entrypoint(name, 
> > "module_init");  
> 
> Why do you keep the fallback for "module_init" here? Modules developed
> against an older weston won't likely work anyway due to other changes,
> so i think we can just jump to the new entrypoint name.

Agreed, but if you do what to keep it, I think it would be nice to have
it log a warning that the module is using an old entrypoint.

> > +   if (!shell_init)
> > +   return -1;
> > +   if (shell_init(compositor, argc, argv) < 0)
> > +   return -1;
> > +   return 0;
> > +}
> > +
> > +WL_EXPORT int
> > +wet_load_module(struct weston_compositor *compositor,
> > +   const char *name, int *argc, char *argv[])
> > +{
> > +   int (*module_init)(struct weston_compositor *ec,
> > +  int *argc, char *argv[]);
> > +
> > +   module_init = wet_load_module_entrypoint(name, "wet_module_init");
> > +   if (!module_init)
> > +   module_init = wet_load_module_entrypoint(name, 
> > "module_init");

The same here.

> > +   if (!module_init)
> > +   return -1;
> > +   if (module_init(compositor, argc, argv) < 0)
> > +   return -1;
> > +   return 0;
> > +}
> > +
> >  static int
> >  load_modules(struct weston_compositor *ec, const char *modules,
> >  int *argc, char *argv[])
> >  {
> > const char *p, *end;
> > char buffer[256];
> > -   int (*module_init)(struct weston_compositor *ec,
> > -  int *argc, char *argv[]);
> >
> > if (modules == NULL)
> > return 0;
> > @@ -763,10 +795,7 @@ load_modules(struct weston_compositor *ec, const char 
> > *modules,
> > end = strchrnul(p, ',');
> > 

Re: [PATCH weston 2/3] modules: Drop module_init as a shared init function

[Giulio Camuffo]

2016-07-04 15:58 GMT+02:00 Quentin Glidic :
> From: Quentin Glidic 
>
> Use different functions so we cannot load a libweston module in weston
> or the other way around.
>
> Also properly namespace backend_init and use a different name for weston
> shells.
>
> Signed-off-by: Quentin Glidic 
> ---
>  compositor/cms-colord.c |  5 +++--
>  compositor/cms-static.c |  4 ++--
>  compositor/main.c   | 45 
> ++---
>  compositor/screen-share.c   |  4 ++--
>  compositor/weston.h | 13 ++-
>  desktop-shell/shell.c   |  4 ++--
>  fullscreen-shell/fullscreen-shell.c |  5 +++--
>  ivi-shell/ivi-layout.c  |  4 +++-
>  ivi-shell/ivi-shell.c   |  4 ++--
>  libweston/compositor-drm.c  |  4 ++--
>  libweston/compositor-fbdev.c|  4 ++--
>  libweston/compositor-headless.c |  4 ++--
>  libweston/compositor-rdp.c  |  4 ++--
>  libweston/compositor-wayland.c  |  4 ++--
>  libweston/compositor-x11.c  |  4 ++--
>  libweston/compositor.c  | 23 ---
>  libweston/compositor.h  |  7 +++---
>  tests/plugin-registry-test.c|  4 +++-
>  tests/surface-global-test.c |  4 +++-
>  tests/surface-screenshot.c  |  5 +++--
>  tests/surface-test.c|  4 +++-
>  tests/weston-test.c |  4 ++--
>  xwayland/launcher.c |  3 +--
>  23 files changed, 111 insertions(+), 55 deletions(-)
>
> diff --git a/compositor/cms-colord.c b/compositor/cms-colord.c
> index b9bc9e3..b9938b9 100644
> --- a/compositor/cms-colord.c
> +++ b/compositor/cms-colord.c
> @@ -33,6 +33,7 @@
>  #include 
>
>  #include "compositor.h"
> +#include "weston.h"
>  #include "cms-helper.h"
>  #include "shared/helpers.h"
>
> @@ -494,8 +495,8 @@ colord_cms_output_destroy(gpointer data)
>  }
>
>  WL_EXPORT int
> -module_init(struct weston_compositor *ec,
> -   int *argc, char *argv[])
> +wet_module_init(struct weston_compositor *ec,
> +   int *argc, char *argv[])
>  {
> gboolean ret;
> GError *error = NULL;
> diff --git a/compositor/cms-static.c b/compositor/cms-static.c
> index a6bbfd4..e24501b 100644
> --- a/compositor/cms-static.c
> +++ b/compositor/cms-static.c
> @@ -91,8 +91,8 @@ cms_notifier_destroy(struct wl_listener *listener, void 
> *data)
>
>
>  WL_EXPORT int
> -module_init(struct weston_compositor *ec,
> -   int *argc, char *argv[])
> +wet_module_init(struct weston_compositor *ec,
> +   int *argc, char *argv[])
>  {
> struct cms_static *cms;
> struct weston_output *output;
> diff --git a/compositor/main.c b/compositor/main.c
> index 4e6b7ae..88f7911 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -704,7 +704,7 @@ weston_create_listening_socket(struct wl_display 
> *display, const char *socket_na
>  }
>
>  WL_EXPORT void *
> -wet_load_module(const char *name, const char *entrypoint)
> +wet_load_module_entrypoint(const char *name, const char *entrypoint)
>  {
> const char *builddir = getenv("WESTON_BUILD_DIR");
> char path[PATH_MAX];
> @@ -746,14 +746,46 @@ wet_load_module(const char *name, const char 
> *entrypoint)
> return init;
>  }
>
> +static int
> +wet_load_shell(struct weston_compositor *compositor,
> +  const char *name, int *argc, char *argv[])
> +{
> +   int (*shell_init)(struct weston_compositor *ec,
> + int *argc, char *argv[]);
> +
> +   shell_init = wet_load_module_entrypoint(name, "wet_shell_init");
> +   if (!shell_init)
> +   shell_init = wet_load_module_entrypoint(name, "module_init");

Why do you keep the fallback for "module_init" here? Modules developed
against an older weston won't likely work anyway due to other changes,
so i think we can just jump to the new entrypoint name.

> +   if (!shell_init)
> +   return -1;
> +   if (shell_init(compositor, argc, argv) < 0)
> +   return -1;
> +   return 0;
> +}
> +
> +WL_EXPORT int
> +wet_load_module(struct weston_compositor *compositor,
> +   const char *name, int *argc, char *argv[])
> +{
> +   int (*module_init)(struct weston_compositor *ec,
> +  int *argc, char *argv[]);
> +
> +   module_init = wet_load_module_entrypoint(name, "wet_module_init");
> +   if (!module_init)
> +   module_init = wet_load_module_entrypoint(name, "module_init");
> +   if (!module_init)
> +   return -1;
> +   if (module_init(compositor, argc, argv) < 0)
> +   return -1;
> +   return 0;
> +}
> +
>  static int
>  load_modules(struct weston_compositor *ec, const char *modules,
>  int *argc, char *argv[])
>  {
> const char *p, *end;
> char 

[PATCH weston 2/3] modules: Drop module_init as a shared init function

[Quentin Glidic]

From: Quentin Glidic 

Use different functions so we cannot load a libweston module in weston
or the other way around.

Also properly namespace backend_init and use a different name for weston
shells.

Signed-off-by: Quentin Glidic 
---
 compositor/cms-colord.c |  5 +++--
 compositor/cms-static.c |  4 ++--
 compositor/main.c   | 45 ++---
 compositor/screen-share.c   |  4 ++--
 compositor/weston.h | 13 ++-
 desktop-shell/shell.c   |  4 ++--
 fullscreen-shell/fullscreen-shell.c |  5 +++--
 ivi-shell/ivi-layout.c  |  4 +++-
 ivi-shell/ivi-shell.c   |  4 ++--
 libweston/compositor-drm.c  |  4 ++--
 libweston/compositor-fbdev.c|  4 ++--
 libweston/compositor-headless.c |  4 ++--
 libweston/compositor-rdp.c  |  4 ++--
 libweston/compositor-wayland.c  |  4 ++--
 libweston/compositor-x11.c  |  4 ++--
 libweston/compositor.c  | 23 ---
 libweston/compositor.h  |  7 +++---
 tests/plugin-registry-test.c|  4 +++-
 tests/surface-global-test.c |  4 +++-
 tests/surface-screenshot.c  |  5 +++--
 tests/surface-test.c|  4 +++-
 tests/weston-test.c |  4 ++--
 xwayland/launcher.c |  3 +--
 23 files changed, 111 insertions(+), 55 deletions(-)

diff --git a/compositor/cms-colord.c b/compositor/cms-colord.c
index b9bc9e3..b9938b9 100644
--- a/compositor/cms-colord.c
+++ b/compositor/cms-colord.c
@@ -33,6 +33,7 @@
 #include 
 
 #include "compositor.h"
+#include "weston.h"
 #include "cms-helper.h"
 #include "shared/helpers.h"
 
@@ -494,8 +495,8 @@ colord_cms_output_destroy(gpointer data)
 }
 
 WL_EXPORT int
-module_init(struct weston_compositor *ec,
-   int *argc, char *argv[])
+wet_module_init(struct weston_compositor *ec,
+   int *argc, char *argv[])
 {
gboolean ret;
GError *error = NULL;
diff --git a/compositor/cms-static.c b/compositor/cms-static.c
index a6bbfd4..e24501b 100644
--- a/compositor/cms-static.c
+++ b/compositor/cms-static.c
@@ -91,8 +91,8 @@ cms_notifier_destroy(struct wl_listener *listener, void *data)
 
 
 WL_EXPORT int
-module_init(struct weston_compositor *ec,
-   int *argc, char *argv[])
+wet_module_init(struct weston_compositor *ec,
+   int *argc, char *argv[])
 {
struct cms_static *cms;
struct weston_output *output;
diff --git a/compositor/main.c b/compositor/main.c
index 4e6b7ae..88f7911 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -704,7 +704,7 @@ weston_create_listening_socket(struct wl_display *display, 
const char *socket_na
 }
 
 WL_EXPORT void *
-wet_load_module(const char *name, const char *entrypoint)
+wet_load_module_entrypoint(const char *name, const char *entrypoint)
 {
const char *builddir = getenv("WESTON_BUILD_DIR");
char path[PATH_MAX];
@@ -746,14 +746,46 @@ wet_load_module(const char *name, const char *entrypoint)
return init;
 }
 
+static int
+wet_load_shell(struct weston_compositor *compositor,
+  const char *name, int *argc, char *argv[])
+{
+   int (*shell_init)(struct weston_compositor *ec,
+ int *argc, char *argv[]);
+
+   shell_init = wet_load_module_entrypoint(name, "wet_shell_init");
+   if (!shell_init)
+   shell_init = wet_load_module_entrypoint(name, "module_init");
+   if (!shell_init)
+   return -1;
+   if (shell_init(compositor, argc, argv) < 0)
+   return -1;
+   return 0;
+}
+
+WL_EXPORT int
+wet_load_module(struct weston_compositor *compositor,
+   const char *name, int *argc, char *argv[])
+{
+   int (*module_init)(struct weston_compositor *ec,
+  int *argc, char *argv[]);
+
+   module_init = wet_load_module_entrypoint(name, "wet_module_init");
+   if (!module_init)
+   module_init = wet_load_module_entrypoint(name, "module_init");
+   if (!module_init)
+   return -1;
+   if (module_init(compositor, argc, argv) < 0)
+   return -1;
+   return 0;
+}
+
 static int
 load_modules(struct weston_compositor *ec, const char *modules,
 int *argc, char *argv[])
 {
const char *p, *end;
char buffer[256];
-   int (*module_init)(struct weston_compositor *ec,
-  int *argc, char *argv[]);
 
if (modules == NULL)
return 0;
@@ -763,10 +795,7 @@ load_modules(struct weston_compositor *ec, const char 
*modules,
end = strchrnul(p, ',');
snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
 
-   module_init = wet_load_module(buffer, "module_init");
-   if (!module_init)
-   return -1;
-