[PATCH v1] refactor configuration API of wayland-backend
Pekka Paalanen
ppaalanen at gmail.com
Mon Apr 25 12:33:37 UTC 2016
On Fri, 22 Apr 2016 15:30:02 +0200
Benoit Gschwind <gschwind at gnu-log.net> wrote:
> Implement a "well" defined API to configure the wayland backend. Following
> according to discution about libweston API.
> ---
> v1:
> - Take care of weston coding style
> - Mimic the old configuration behaviour
> - Front declaration of variable
> - Static allocation of configuration structure
> - Using single zalloc for the output list allocation
> v0:
> - Initial draft
>
> Signed-off-by: Benoit Gschwind <gschwind at gnu-log.net>
Hi Benoit,
thanks for taking care of this.
I feel that there is very much going on in this patch. Could you see if
you can split it into smaller, logical patches? E.g.:
- initial clean-ups
- main refactoring
- adding new checks
- adding new names
- etc.
I hope my comments below give more hints on what could be a good split.
Do not use line count as a measure, but do it by topics. But if you
can't find a good split, then nevermind.
>
> Makefile.am | 1 +
> src/compositor-wayland.c | 202 +++++++++++++-------------------------------
> src/compositor-wayland.h | 65 +++++++++++++++
> src/main.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 335 insertions(+), 145 deletions(-)
> create mode 100644 src/compositor-wayland.h
>
> diff --git a/Makefile.am b/Makefile.am
> index c042c68..e94ffa1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -73,6 +73,7 @@ weston_SOURCES = \
> src/compositor.c \
> src/compositor.h \
> src/compositor-headless.h \
> + src/compositor-wayland.h \
> src/input.c \
> src/data-device.c \
> src/screenshooter.c \
The same comments as before for other backends, missing two more places
to add the header to: the backend sources and installed headers. See
the patch for headless in upstream master branch.
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 9d1a251..db6456e 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -40,6 +40,7 @@
> #include <wayland-cursor.h>
>
> #include "compositor.h"
> +#include "compositor-wayland.h"
> #include "gl-renderer.h"
> #include "pixman-renderer.h"
> #include "shared/helpers.h"
> @@ -79,6 +80,7 @@ struct wayland_backend {
> struct wl_cursor *cursor;
>
> struct wl_list input_list;
> +
Spurious change.
> };
>
> struct wayland_output {
> @@ -1091,65 +1093,6 @@ err_name:
> }
>
> static struct wayland_output *
> -wayland_output_create_for_config(struct wayland_backend *b,
> - struct weston_config_section *config_section,
> - int option_width, int option_height,
> - int option_scale, int32_t x, int32_t y)
> -{
> - struct wayland_output *output;
> - char *mode, *t, *name, *str;
> - int width, height, scale;
> - uint32_t transform;
> - unsigned int slen;
> -
> - weston_config_section_get_string(config_section, "name", &name, NULL);
> - if (name) {
> - slen = strlen(name);
> - slen += strlen(WINDOW_TITLE " - ");
> - str = malloc(slen + 1);
> - if (str)
> - snprintf(str, slen + 1, WINDOW_TITLE " - %s", name);
> - free(name);
> - name = str;
> - }
> - if (!name)
> - name = strdup(WINDOW_TITLE);
> -
> - weston_config_section_get_string(config_section,
> - "mode", &mode, "1024x600");
> - if (sscanf(mode, "%dx%d", &width, &height) != 2) {
> - weston_log("Invalid mode \"%s\" for output %s\n",
> - mode, name);
> - width = 1024;
> - height = 640;
> - }
> - free(mode);
> -
> - if (option_width)
> - width = option_width;
> - if (option_height)
> - height = option_height;
> -
> - weston_config_section_get_int(config_section, "scale", &scale, 1);
> -
> - if (option_scale)
> - scale = option_scale;
> -
> - weston_config_section_get_string(config_section,
> - "transform", &t, "normal");
> - if (weston_parse_transform(t, &transform) < 0)
> - weston_log("Invalid transform \"%s\" for output %s\n",
> - t, name);
> - free(t);
> -
> - output = wayland_output_create(b, x, y, width, height, name, 0,
> - transform, scale);
> - free(name);
> -
> - return output;
> -}
I see that whole function go away (particularly the WINDOW_TITLE
stuff), I wonder what replaces it, and I don't seem to find anything.
Was this function already redundant? If that was so, removing the
redundancy could be a separate patch.
> -
> -static struct wayland_output *
> wayland_output_create_for_parent_output(struct wayland_backend *b,
> struct wayland_parent_output *poutput)
> {
> @@ -2167,25 +2110,18 @@ static const char *left_ptrs[] = {
> };
>
> static void
> -create_cursor(struct wayland_backend *b, struct weston_config *config)
> +create_cursor(struct wayland_backend *b,
> + struct weston_wayland_backend_config *config)
> {
> - struct weston_config_section *s;
> - int size;
> - char *theme = NULL;
> unsigned int i;
>
> - s = weston_config_get_section(config, "shell", NULL, NULL);
> - weston_config_section_get_string(s, "cursor-theme", &theme, NULL);
> - weston_config_section_get_int(s, "cursor-size", &size, 32);
> -
> - b->cursor_theme = wl_cursor_theme_load(theme, size, b->parent.shm);
> + b->cursor_theme = wl_cursor_theme_load(config->cursor_theme,
> + config->cursor_size, b->parent.shm);
> if (!b->cursor_theme) {
> fprintf(stderr, "could not load cursor theme\n");
> return;
> }
>
> - free(theme);
> -
> b->cursor = NULL;
> for (i = 0; !b->cursor && i < ARRAY_LENGTH(left_ptrs); ++i)
> b->cursor = wl_cursor_theme_get_cursor(b->cursor_theme,
> @@ -2219,9 +2155,8 @@ fullscreen_binding(struct weston_keyboard *keyboard, uint32_t time,
> }
>
> static struct wayland_backend *
> -wayland_backend_create(struct weston_compositor *compositor, int use_pixman,
> - const char *display_name, int *argc, char *argv[],
> - struct weston_config *config)
> +wayland_backend_create(struct weston_compositor *compositor,
> + struct weston_wayland_backend_config *config)
> {
> struct wayland_backend *b;
> struct wl_event_loop *loop;
> @@ -2235,7 +2170,7 @@ wayland_backend_create(struct weston_compositor *compositor, int use_pixman,
> if (weston_compositor_set_presentation_clock_software(compositor) < 0)
> goto err_compositor;
>
> - b->parent.wl_display = wl_display_connect(display_name);
> + b->parent.wl_display = wl_display_connect(config->display_name);
> if (b->parent.wl_display == NULL) {
> weston_log("failed to create display: %m\n");
> goto err_compositor;
> @@ -2249,7 +2184,7 @@ wayland_backend_create(struct weston_compositor *compositor, int use_pixman,
>
> create_cursor(b, config);
>
> - b->use_pixman = use_pixman;
> + b->use_pixman = config->use_pixman;
>
> if (!b->use_pixman) {
> gl_renderer = weston_load_module("gl-renderer.so",
> @@ -2320,50 +2255,44 @@ wayland_backend_destroy(struct wayland_backend *b)
> wl_cursor_theme_destroy(b->cursor_theme);
>
> weston_compositor_shutdown(b->compositor);
> +
> free(b);
> }
>
> +static void
> +config_init_to_defaults(struct weston_wayland_backend_config *config)
> +{
> +}
> +
> WL_EXPORT int
> -backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config,
> +backend_init(struct weston_compositor *compositor,
> + int *argc, char *argv[],
> + struct weston_config *wc,
> struct weston_backend_config *config_base)
> {
> struct wayland_backend *b;
> - struct wayland_output *output;
> + struct weston_wayland_backend_config config = {{ 0, }};
> struct wayland_parent_output *poutput;
> - struct weston_config_section *section;
> - int x, count, width, height, scale, use_pixman, fullscreen, sprawl;
> - const char *section_name, *display_name;
> - char *name;
> + struct wayland_output *output;
> + struct weston_wayland_backend_output_config const *o;
> + int x;
> + uint32_t i;
> +
> + if (config_base == NULL ||
> + config_base->struct_version != WESTON_WAYLAND_BACKEND_CONFIG_VERSION ||
> + config_base->struct_size > sizeof(struct weston_wayland_backend_config)) {
> + weston_log("wayland backend config structure is invalid\n");
> + return -1;
> + }
> +
> + config_init_to_defaults(&config);
> + memcpy(&config, config_base, config_base->struct_size);
>
> - const struct weston_option wayland_options[] = {
> - { WESTON_OPTION_INTEGER, "width", 0, &width },
> - { WESTON_OPTION_INTEGER, "height", 0, &height },
> - { WESTON_OPTION_INTEGER, "scale", 0, &scale },
> - { WESTON_OPTION_STRING, "display", 0, &display_name },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &use_pixman },
> - { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> - { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen },
> - { WESTON_OPTION_BOOLEAN, "sprawl", 0, &sprawl },
> - };
> -
> - width = 0;
> - height = 0;
> - scale = 0;
> - display_name = NULL;
> - use_pixman = 0;
> - count = 1;
> - fullscreen = 0;
> - sprawl = 0;
> - parse_options(wayland_options,
> - ARRAY_LENGTH(wayland_options), argc, argv);
> -
> - b = wayland_backend_create(compositor, use_pixman, display_name,
> - argc, argv, config);
> - if (!b)
> + b = wayland_backend_create(compositor, &config);
> + if (b == NULL)
> return -1;
>
> - if (sprawl || b->parent.fshell) {
> + if (config.sprawl || b->parent.fshell) {
> b->sprawl_across_outputs = 1;
> wl_display_roundtrip(b->parent.wl_display);
>
> @@ -2373,58 +2302,44 @@ backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> return 0;
> }
>
> - if (fullscreen) {
> - output = wayland_output_create(b, 0, 0, width, height,
> - NULL, 1, 0, 1);
> + if (config.fullscreen) {
> + if (config.num_outputs != 1)
> + goto err_outputs;
> +
> + o = &config.outputs[0];
> + /* should not append */
"append"?
> + if (o->name == NULL)
> + goto err_outputs;
> +
> + output = wayland_output_create(b, 0, 0, o->width, o->height,
> + o->name, 1, o->transform, o->scale);
> +
> if (!output)
> goto err_outputs;
>
> wayland_output_set_fullscreen(output, 0, 0, NULL);
> +
> return 0;
> +
> }
>
> - section = NULL;
> x = 0;
> - while (weston_config_next_section(config, §ion, §ion_name)) {
> - if (!section_name || strcmp(section_name, "output") != 0)
> - continue;
> - weston_config_section_get_string(section, "name", &name, NULL);
> - if (name == NULL)
> + for (i = 0; i < config.num_outputs; ++i) {
> + o = &config.outputs[i];
> + /* should not append */
"append"?
> + if (o->name == NULL)
> continue;
>
> - if (name[0] != 'W' || name[1] != 'L') {
> - free(name);
> - continue;
> - }
> - free(name);
> + output = wayland_output_create(b, x, 0, o->width, o->height,
> + o->name, 0, o->transform, o->scale);
>
> - output = wayland_output_create_for_config(b, section, width,
> - height, scale, x, 0);
> if (!output)
> goto err_outputs;
> - if (wayland_output_set_windowed(output))
> - goto err_outputs;
>
> - x += output->base.width;
> - --count;
> - }
> -
> - if (!width)
> - width = 1024;
> - if (!height)
> - height = 640;
> - if (!scale)
> - scale = 1;
> - while (count > 0) {
> - output = wayland_output_create(b, x, 0, width, height,
> - NULL, 0, 0, scale);
> - if (!output)
> - goto err_outputs;
> if (wayland_output_set_windowed(output))
> goto err_outputs;
>
> - x += width;
> - --count;
> + x += o->width;
> }
>
> weston_compositor_add_key_binding(compositor, KEY_F,
> @@ -2437,3 +2352,4 @@ err_outputs:
> wayland_backend_destroy(b);
> return -1;
> }
> +
> diff --git a/src/compositor-wayland.h b/src/compositor-wayland.h
> new file mode 100644
> index 0000000..f7e9ccd
> --- /dev/null
> +++ b/src/compositor-wayland.h
> @@ -0,0 +1,65 @@
> +/*
> + * Copyright © 2016 Benoit Gschwind
> + *
> + * 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 (including the
> + * next paragraph) 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.
> + */
> +
> +#ifndef SRC_COMPOSITOR_WAYLAND_H_
> +#define SRC_COMPOSITOR_WAYLAND_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include "compositor.h"
> +
> +#define WESTON_WAYLAND_BACKEND_CONFIG_VERSION 1
> +
> +struct weston_wayland_backend_output_config {
> + int width;
> + int height;
> + char *name;
> + uint32_t transform;
> + int32_t scale;
> +};
> +
> +struct weston_wayland_backend_config {
> + struct weston_backend_config base;
> +
> + int use_pixman;
> + int sprawl;
> + char *display_name;
> +
> + char *cursor_theme;
> + int cursor_size;
> +
> + int fullscreen;
> +
> + uint32_t num_outputs;
> + struct weston_wayland_backend_output_config *outputs;
This I think is fine. The Wayland backend does not have hardware
hotplug as usual. Outputs that are hotplugged come with their own
configuration from the parent compositor, so there is no need for a
callback. It is enough to use an array created that startup time.
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* SRC_COMPOSITOR_WAYLAND_H_ */
> diff --git a/src/main.c b/src/main.c
> index fc97dd8..31f5296 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -42,6 +42,7 @@
> #endif
>
> #include "compositor.h"
> +#include "compositor-wayland.h"
> #include "../shared/os-compatibility.h"
> #include "../shared/helpers.h"
> #include "git-version.h"
> @@ -715,6 +716,213 @@ load_headless_backend(struct weston_compositor *c, char const * backend,
> ret = load_backend_new(c, backend, &config.base);
>
> return ret;
> +
> +}
> +
> +static int
> +load_wayland_backend(struct weston_compositor *c, char const * backend,
> + int *argc, char **argv, struct weston_config *wc)
> +{
> + struct weston_config_section *section;
> + struct weston_wayland_backend_output_config * output;
Don't use a space after the asterisk here.
> + int count;
> + int option_width;
> + int option_height;
> + int option_scale;
> + int fullscreen;
> + int width;
> + int height;
> + int scale;
> + const char *section_name;
> + char screen_name[32];
> + char *name;
> + char *transform_name;
> + char *mode;
> + int ret = 0;
> + uint32_t i;
> + uint32_t transform;
> +
> + struct weston_wayland_backend_config config = {{ 0, }};
> +
> + const struct weston_option wayland_options[] = {
> + { WESTON_OPTION_INTEGER, "width", 0, &option_width },
> + { WESTON_OPTION_INTEGER, "height", 0, &option_height },
> + { WESTON_OPTION_INTEGER, "scale", 0, &option_scale },
> + { WESTON_OPTION_STRING, "display", 0, &config.display_name },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> + { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> + { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen },
> + { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config.sprawl },
> + };
> +
> + option_width = 0;
> + option_height = 0;
> + option_scale = 0;
> + count = 1;
> + fullscreen = 0;
> + parse_options(wayland_options,
> + ARRAY_LENGTH(wayland_options), argc, argv);
> +
> + config.base.struct_version = WESTON_WAYLAND_BACKEND_CONFIG_VERSION;
> + config.base.struct_size = sizeof(struct weston_wayland_backend_config);
> +
> + config.fullscreen = fullscreen;
> +
> + section = weston_config_get_section(wc, "shell", NULL, NULL);
> + weston_config_section_get_string(section, "cursor-theme",
> + &config.cursor_theme, NULL);
> + weston_config_section_get_int(section, "cursor-size", &config.cursor_size,
> + 32);
If you wrap a long line, wrap it below 80 chars and align the
continuation after the opening parenthesis:
https://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n159
> +
> + if (fullscreen) {
> +
An empty line too much.
> + if (!option_width) {
> + weston_log("invalid width\n");
> + ret = -1;
> + goto out;
> + }
> +
> + if (!option_height) {
> + weston_log("invalid height\n");
> + ret = -1;
> + goto out;
> + }
These seem to be new checks. They are correct, but should be mentioned
in the commit message as new additions.
> +
> + config.num_outputs = 1;
> + config.outputs =
> + (struct weston_wayland_backend_output_config *)
> + zalloc(sizeof(struct weston_wayland_backend_output_config));
Do not cast the return value of alloc() functions. We have void* this
day and age which casts implicitly.
> +
> + if (!config.outputs) {
> + weston_log("Fail to allocate outputs configuration data\n");
> + ret = -1;
> + goto out;
> + }
> +
> + output = &config.outputs[0];
> +
> + snprintf(screen_name, 32, "screen%d", count);
Did this name exist before? I don't see it, is it a new addition?
> +
> + output->width = option_width;
> + output->height = option_height;
> + output->name = strdup(name);
asprintf() would do the same easier.
> + output->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> + output->scale = 1;
> +
> + } else {
> + if (count <= 0) {
> + weston_log(
> + "Invalid output-count \"%d\", must be non null and positive\n",
> + count);
That's a strange line formatting.
> + ret = -1;
> + goto out;
> + }
> +
> + config.num_outputs = count;
> + config.outputs =
> + (struct weston_wayland_backend_output_config *)
Do not cast.
> + zalloc(count*sizeof(struct weston_wayland_backend_output_config));
The operator would need spaces around it, but use calloc() instead.
Calloc checks for overflows in the multiplication.
> +
> + if (!config.outputs) {
> + weston_log("Fail to allocate outputs configuration data\n");
> + ret = -1;
> + goto out;
> + }
> +
> + section = NULL;
> + while (weston_config_next_section(wc, §ion, §ion_name)) {
> + output = &config.outputs[config.num_outputs-count];
Binary operator needs spaces around it.
> + if (!section_name || strcmp(section_name, "output") != 0)
> + continue;
> + weston_config_section_get_string(section, "name", &name, NULL);
> + if (name == NULL)
> + continue;
> +
> + if (name[0] != 'W' || name[1] != 'L') {
> + free(name);
> + continue;
> + }
> +
> + /* setup the width, the height, the scale and the transform of
> + * this output. */
> +
> + transform = WL_OUTPUT_TRANSFORM_NORMAL;
> +
> + weston_config_section_get_string(section, "mode", &mode,
> + "1024x600");
> + if (sscanf(mode, "%dx%d", &width, &height) != 2) {
> + weston_log("Invalid mode \"%s\" for output %s\n",
> + mode, name);
> + width = 1024;
> + height = 640;
> + }
> + free(mode);
> +
> + if (option_width)
> + width = option_width;
> + if (option_height)
> + width = option_height;
> +
> + weston_config_section_get_int(section, "scale", &scale, 1);
> +
> + if (option_scale)
> + scale = option_scale;
> +
> + weston_config_section_get_string(section,
> + "transform", &transform_name, "normal");
> + if (weston_parse_transform(transform_name, &transform) < 0)
> + weston_log("Invalid transform \"%s\" for output %s\n",
> + transform_name, name);
> + free(transform_name);
> +
> + output->width = width;
> + output->height = height;
> + output->name = strdup(name);
> + output->transform = transform;
> + output->scale = scale;
> +
> + free(name);
> +
> + --count;
> + }
> +
> + /* fill remaining outputs */
> + if (!option_width)
> + option_width = 1024;
> + if (!option_height)
> + option_height = 640;
> + if (!option_scale)
> + option_scale = 1;
> + while (count > 0) {
> + output = &config.outputs[config.num_outputs-count];
Binary operator needs spaces.
> +
> + snprintf(screen_name, 32, "screen%d", count);
Is this name a new invention? Refactoring patches should not introduce
anything new, or at least should list them in the commit message.
Isn't the numbering a bit backwards? Not that it'd make a difference.
> +
> + output->width = option_width;
> + output->height = option_height;
> + output->name = strdup(name);
> + output->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> + output->scale = option_scale;
> +
> + --count;
> + }
> +
> + }
Makes me wonder if there was something to split into functions in those
long statement blocks.
> +
> + /* load the actual wayland backend and configure it */
> + ret = load_backend_new(c, backend, &config.base);
> +
> +out:
> +
> + /** cleanup **/
> + if (config.outputs) {
> + for (i = 0; i < config.num_outputs; ++i) {
> + free(config.outputs[i].name);
> + }
> + free(config.outputs);
> + }
> +
> + return ret;
> }
>
> static int
> @@ -723,11 +931,11 @@ load_backend(struct weston_compositor *compositor, const char *backend,
> {
> if (strstr(backend, "headless-backend.so"))
> return load_headless_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "wayland-backend.so"))
> + return load_wayland_backend(compositor, backend, argc, argv, config);
> #if 0
> else if (strstr(backend, "drm-backend.so"))
> return load_drm_backend(compositor, backend, argc, argv, config);
> - else if (strstr(backend, "wayland-backend.so"))
> - return load_wayland_backend(compositor, backend, argc, argv, config);
> else if (strstr(backend, "x11-backend.so"))
> return load_x11_backend(compositor, backend, argc, argv, config);
> else if (strstr(backend, "fbdev-backend.so"))
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160425/6ced5fb0/attachment-0001.sig>
More information about the wayland-devel
mailing list