[PATCH weston 2/3] modules: Drop module_init as a shared init function
Pekka Paalanen
ppaalanen at gmail.com
Fri Dec 2 12:48:08 UTC 2016
On Mon, 11 Jul 2016 11:05:38 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 2016-07-04 15:58 GMT+02:00 Quentin Glidic <sardemff7+wayland at sardemff7.net>:
> > From: Quentin Glidic <sardemff7+git at sardemff7.net>
> >
> > 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 <sardemff7+git at sardemff7.net>
> > ---
> > 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, ',');
> > snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
> >
> > - module_init = wet_load_module(buffer, "module_init");
> > - if (!module_init)
> > - return -1;
> > - if (module_init(ec, argc, argv) < 0)
> > + if (wet_load_module(ec, buffer, argc, argv) < 0)
> > return -1;
> > p = end;
> > while (*p == ',')
> > @@ -1714,7 +1743,7 @@ int main(int argc, char *argv[])
> > goto out;
> > }
> >
> > - if (load_modules(ec, shell, &argc, argv) < 0)
> > + if (wet_load_shell(ec, shell, &argc, argv) < 0)
> > goto out;
> >
> > weston_config_section_get_string(section, "modules", &modules, "");
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index 771f1c9..8f30b46 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -5023,7 +5023,9 @@ weston_compositor_load_backend(struct weston_compositor *compositor,
> > if (backend < 0 || backend >= ARRAY_LENGTH(backend_map))
> > return -1;
> >
> > - backend_init = weston_load_module(backend_map[backend], "backend_init");
> > + backend_init = weston_load_module(backend_map[backend], "weston_backend_init");
> > + if (!backend_init)
> > + backend_init = weston_load_module(backend_map[backend], "backend_init");
This fallback is not necessary, we don't even pretend to support
external backends.
> > if (!backend_init)
> > return -1;
> >
> > @@ -5033,14 +5035,19 @@ weston_compositor_load_backend(struct weston_compositor *compositor,
> > WL_EXPORT int
> > weston_compositor_load_xwayland(struct weston_compositor *compositor)
> > {
> > - int (*module_init)(struct weston_compositor *ec,
> > - int *argc, char *argv[]);
> > - int argc = 0;
> > + int (*weston_module_init)(struct weston_compositor *ec);
> >
> > - module_init = weston_load_module("xwayland.so", "module_init");
> > - if (!module_init)
> > - return -1;
> > - if (module_init(compositor, &argc, NULL) < 0)
> > + weston_module_init = weston_load_module("xwayland.so", "weston_module_init");
> > + if (!weston_module_init) {
> > + int (*module_init)(struct weston_compositor *ec,
> > + int *argc, char *argv[]);
> > + int argc = 0;
> > +
> > + module_init = weston_load_module("xwayland.so", "module_init");
> > + if (!module_init || module_init(compositor, &argc, NULL) < 0)
> > + return -1;
I think this fallback mode to the old name is unnecessary. This is
libweston loading a specific libweston plugin and we always assume they
match.
> > + } else if (weston_module_init(compositor) < 0) {
> > return -1;
> > + }
> > return 0;
> > }
Other than the above comments, this patch would be ready to land. Nice
work.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161202/49e6db1d/attachment.sig>
More information about the wayland-devel
mailing list