On Mon, 4 Jul 2016 15:58:09 +0200 Quentin Glidic <[email protected]> wrote:
> From: Quentin Glidic <[email protected]> > > 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 <[email protected]> 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
pgp_PICQ7U6gW.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
