[PATCH weston 3/3] libweston: Add a generic weston_compositor_load_module
Pekka Paalanen
ppaalanen at gmail.com
Fri Dec 2 14:38:24 UTC 2016
On Mon, 4 Jul 2016 15:58:09 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:
> From: Quentin Glidic <sardemff7+git at sardemff7.net>
>
> This way, we can share modules between libweston-based compositors, but
> they can only be loaded explicitely by the compositor.
>
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
Hi,
this seems like a fairly big step: we are introducing the concept of
"third-party libweston plugins", freely installable and loadable by
anyone. I wonder if they should be clearly separated from libweston
internal modules like xwayland.so, since libweston will (should?)
always match the revision of internal plugins.
You punt the responsibility of ensuring the compatiblity with the
compositor to the compositor. Examples of public libweston plugins
(provided by weston rather than libweston) are cms-colord.so and
screenshare.so, as you mention those in the manual.
You leave cms-static.so out (intentionally) since it does not apply: it
uses wet_get_config() which libweston does not provide.
It's very good to have these example of what kind of plugins could be
so generic that they don't require much anything from the compositor.
Third-party plugins could also use the plugin registry to offer new
APIs for the compositor to use.
Ok.
What is the difference between, say, Weston plugins and libweston
plugins?
I think we should have a section in the README about plugins explaining
all the above. Right now there is just "plugin design ???" but it seems
the design is forming. Then it would be easier to see and discuss what
this means.
> ---
> Makefile.am | 3 ++-
> compositor/main.c | 38 ++++++++++++++++++++++++++++++++++++++
> libweston/compositor.c | 13 ++++++++++---
> libweston/compositor.h | 4 ++++
> man/weston.ini.man | 8 +++++++-
> man/weston.man | 6 ++++++
> 6 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 436bb1b..d99437a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1414,7 +1414,8 @@ endif
>
> MAN_SUBSTS = \
> -e 's|__weston_native_backend__|$(WESTON_NATIVE_BACKEND)|g' \
> - -e 's|__weston_modules_dir__|$(pkglibdir)|g' \
> + -e 's|__libweston_modules_dir__|$(libweston_moduledir)|g' \
> + -e 's|__weston_modules_dir__|$(moduledir)|g' \
> -e 's|__weston_shell_client__|$(WESTON_SHELL_CLIENT)|g' \
> -e 's|__version__|$(PACKAGE_VERSION)|g'
>
> diff --git a/compositor/main.c b/compositor/main.c
> index 88f7911..ea89e47 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -780,6 +780,33 @@ wet_load_module(struct weston_compositor *compositor,
> return 0;
> }
>
> +static int
> +load_cmodules(struct weston_compositor *ec, const char *modules,
> + int *argc, char *argv[])
> +{
> + const char *p, *end;
> + char buffer[256];
> +
> + if (modules == NULL)
> + return 0;
> +
> + p = modules;
> + while (*p) {
> + end = strchrnul(p, ',');
> + snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
> +
> + if (weston_compositor_load_module(ec, buffer) < 0)
> + return -1;
> +
> + p = end;
> + while (*p == ',')
> + p++;
> +
> + }
> +
> + return 0;
> +}
> +
> static int
> load_modules(struct weston_compositor *ec, const char *modules,
> int *argc, char *argv[])
> @@ -1589,7 +1616,9 @@ int main(int argc, char *argv[])
> char *backend = NULL;
> char *shell = NULL;
> int32_t xwayland = 0;
> + char *cmodules = NULL;
> char *modules = NULL;
> + char *option_cmodules = NULL;
> char *option_modules = NULL;
> char *log = NULL;
> char *server_socket = NULL, *end;
> @@ -1612,6 +1641,7 @@ int main(int argc, char *argv[])
> { WESTON_OPTION_STRING, "socket", 'S', &socket_name },
> { WESTON_OPTION_INTEGER, "idle-time", 'i', &idle_time },
> { WESTON_OPTION_BOOLEAN, "xwayland", 0, &xwayland },
> + { WESTON_OPTION_STRING, "common-modules", 0, &option_cmodules },
> { WESTON_OPTION_STRING, "modules", 0, &option_modules },
Could we use just a single entry for all modules? Try libweston module
by the name first, then weston module?
It might be hard for users to see the difference.
> { WESTON_OPTION_STRING, "log", 0, &log },
> { WESTON_OPTION_BOOLEAN, "help", 'h', &help },
> @@ -1746,6 +1776,14 @@ int main(int argc, char *argv[])
> if (wet_load_shell(ec, shell, &argc, argv) < 0)
> goto out;
>
> + weston_config_section_get_string(section, "common-modules", &cmodules,
> + "");
> + if (load_cmodules(ec, cmodules, &argc, argv) < 0)
> + goto out;
> +
> + if (load_cmodules(ec, option_cmodules, &argc, argv) < 0)
> + goto out;
> +
> weston_config_section_get_string(section, "modules", &modules, "");
> if (load_modules(ec, modules, &argc, argv) < 0)
> goto out;
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index 8f30b46..0ed3b0c 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -5033,17 +5033,18 @@ weston_compositor_load_backend(struct weston_compositor *compositor,
> }
>
> WL_EXPORT int
> -weston_compositor_load_xwayland(struct weston_compositor *compositor)
> +weston_compositor_load_module(struct weston_compositor *compositor,
> + const char *name)
> {
> int (*weston_module_init)(struct weston_compositor *ec);
>
> - weston_module_init = weston_load_module("xwayland.so", "weston_module_init");
> + weston_module_init = weston_load_module(name, "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");
> + module_init = weston_load_module(name, "module_init");
> if (!module_init || module_init(compositor, &argc, NULL) < 0)
> return -1;
> } else if (weston_module_init(compositor) < 0) {
> @@ -5051,3 +5052,9 @@ weston_compositor_load_xwayland(struct weston_compositor *compositor)
> }
> return 0;
> }
> +
> +WL_EXPORT int
> +weston_compositor_load_xwayland(struct weston_compositor *compositor)
> +{
> + return weston_compositor_load_module(compositor, "xwayland.so");
This looks a bit awkward to me. Nothing prevents the compositor from
loading xwayland.so by calling weston_compositor_load_module(). Is that
ever useful? Maybe it would be better to make it not work?
> +}
> diff --git a/libweston/compositor.h b/libweston/compositor.h
> index 3fa9b02..827af8a 100644
> --- a/libweston/compositor.h
> +++ b/libweston/compositor.h
> @@ -1723,6 +1723,10 @@ void
> weston_seat_set_keyboard_focus(struct weston_seat *seat,
> struct weston_surface *surface);
>
> +
> +int
> +weston_compositor_load_module(struct weston_compositor *compositor, const char *name);
> +
> int
> weston_compositor_load_xwayland(struct weston_compositor *compositor);
>
> diff --git a/man/weston.ini.man b/man/weston.ini.man
> index 1b1e05a..033b4b0 100644
> --- a/man/weston.ini.man
> +++ b/man/weston.ini.man
> @@ -110,6 +110,12 @@ directory are:
> ask Weston to load the XWayland module (boolean).
> .RE
> .TP 7
> +.BI "common-modules=" cms-colord.so,screen-share.so
> +specifies the modules to load (string). These are shared with other
> +libweston-based compositors. The files are searched for in the
> +.IR "__libweston_modules_dir__"
> +directory.
Maybe this could be reworded. It sounds like loading makes the modules
shared.
> +.TP 7
> .BI "modules=" cms-colord.so,screen-share.so
> specifies the modules to load (string). Available modules in the
> .IR "__weston_modules_dir__"
> @@ -124,7 +130,7 @@ directory are:
> .TP 7
> .BI "backend=" headless-backend.so
> overrides defaults backend. Available backend modules in the
> -.IR "__weston_modules_dir__"
> +.IR "__libweston_modules_dir__"
> directory are:
> .PP
> .RS 10
> diff --git a/man/weston.man b/man/weston.man
> index face229..5338b66 100644
> --- a/man/weston.man
> +++ b/man/weston.man
> @@ -146,6 +146,12 @@ instead of writing them to stderr.
> \fB\-\-xwayland\fR
> Ask Weston to load the XWayland module.
> .TP
> +\fB\-\-common-modules\fR=\fImodule1.so,module2.so\fR
> +Load the comma-separated list of modules shared with other
> +libweston-based compositors. The file is searched for in
> +.IR "__libweston_modules_dir__" ,
> +or you can pass an absolute path.
> +.TP
> \fB\-\-modules\fR=\fImodule1.so,module2.so\fR
> Load the comma-separated list of modules. Only used by the test
> suite. The file is searched for in
Updating README is the biggest issue here, defining what the different
kinds of plugins are, what they can use, and so on.
There is also the nasty question of versioning. The libweston major is
explicit in the module search path, but how are modules themselves
versioned? Through the plugin registry perhaps? Is that sufficient?
Libweston won't be bumping major forever, so are major bumps on plugins
realized by changing the .so name? Should everyone put a major version
number in the plugin name from the start? How does that interact with
libweston major changing?
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/9ba28d6b/attachment-0001.sig>
More information about the wayland-devel
mailing list