Hello pq, Thanks for your review.
I did a patch update. About the initialization and memory leak of .device options, as we discussed on IRC, the current code is valid :) because of the following lines : + if (!config.device) + config.device = strdup("/dev/fb0"); Thus I guess I do not need to fix it :) If there is some other things to improve this let me know :) Best regards. Le 26/04/2016 10:49, Pekka Paalanen a écrit : > On Sat, 23 Apr 2016 11:14:27 +0200 > Benoit Gschwind <gschw...@gnu-log.net> wrote: > >> Implement a "well" defined API to configure the fbdev backend. >> Following according to discution about libweston API >> --- >> v0: >> - First proposal. >> >> src/compositor-fbdev.c | 45 +++++++++++++++++++++++---------------------- >> src/compositor-fbdev.h | 49 >> +++++++++++++++++++++++++++++++++++++++++++++++++ >> src/main.c | 33 +++++++++++++++++++++++++++++++-- >> 3 files changed, 103 insertions(+), 24 deletions(-) >> create mode 100644 src/compositor-fbdev.h > > Hi Benoit, > > this patch is missing all Makefile.am changes. That means it fails distcheck > with > ../../src/main.c:50:30: fatal error: compositor-fbdev.h: No such file or > directory > #include "compositor-fbdev.h" > > You also forgot Signed-off-by. > >> >> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c >> index 19c5e3b..d0597a6 100644 >> --- a/src/compositor-fbdev.c >> +++ b/src/compositor-fbdev.c >> @@ -44,6 +44,7 @@ >> >> #include "shared/helpers.h" >> #include "compositor.h" >> +#include "compositor-fbdev.h" >> #include "launcher-util.h" >> #include "pixman-renderer.h" >> #include "libinput-seat.h" >> @@ -93,12 +94,6 @@ struct fbdev_output { >> uint8_t depth; >> }; >> >> -struct fbdev_parameters { >> - int tty; >> - char *device; >> - int use_gl; >> -}; >> - >> struct gl_renderer_interface *gl_renderer; >> >> static const char default_seat[] = "seat0"; >> @@ -744,7 +739,7 @@ fbdev_restore(struct weston_compositor *compositor) >> static struct fbdev_backend * >> fbdev_backend_create(struct weston_compositor *compositor, int *argc, char >> *argv[], >> struct weston_config *config, >> - struct fbdev_parameters *param) >> + struct weston_fbdev_backend_config *param) >> { >> struct fbdev_backend *backend; >> const char *seat_id = default_seat; >> @@ -827,29 +822,35 @@ out_compositor: >> return NULL; >> } >> >> +static void >> +config_init_to_defaults(struct weston_fbdev_backend_config *config) >> +{ >> + /* TODO: Ideally, available frame buffers should be enumerated using >> + * udev, rather than passing a device node in as a parameter. */ >> + config->tty = 0; /* default to current tty */ >> + config->device = "/dev/fb0"; /* default frame buffer */ >> + config->use_gl = 0; >> +} >> + >> WL_EXPORT int >> backend_init(struct weston_compositor *compositor, int *argc, char *argv[], >> - struct weston_config *config, >> + struct weston_config *wc, >> struct weston_backend_config *config_base) >> { >> struct fbdev_backend *b; >> - /* TODO: Ideally, available frame buffers should be enumerated using >> - * udev, rather than passing a device node in as a parameter. */ >> - struct fbdev_parameters param = { >> - .tty = 0, /* default to current tty */ >> - .device = "/dev/fb0", /* default frame buffer */ >> - .use_gl = 0, >> - }; >> + struct weston_fbdev_backend_config config = {{ 0, }}; >> >> - const struct weston_option fbdev_options[] = { >> - { WESTON_OPTION_INTEGER, "tty", 0, ¶m.tty }, >> - { WESTON_OPTION_STRING, "device", 0, ¶m.device }, >> - { WESTON_OPTION_BOOLEAN, "use-gl", 0, ¶m.use_gl }, >> - }; >> + if (config_base == NULL || >> + config_base->struct_version != WESTON_FBDEV_BACKEND_CONFIG_VERSION >> || >> + config_base->struct_size > sizeof(struct >> weston_fbdev_backend_config)) { >> + weston_log("fbdev backend config structure is invalid\n"); >> + return -1; >> + } >> >> - parse_options(fbdev_options, ARRAY_LENGTH(fbdev_options), argc, argv); >> + config_init_to_defaults(&config); >> + memcpy(&config, config_base, config_base->struct_size); >> >> - b = fbdev_backend_create(compositor, argc, argv, config, ¶m); >> + b = fbdev_backend_create(compositor, argc, argv, wc, &config); > > There is a small issue with config.device. It is ok for > config_init_to_defaults() to use a statically allocated const string > for it, because the pointer gets overwritten with a pointer from > main.c. However, fbdev_output_create() is storing the pointer instead > of calling strdup(). When main.c after init frees the pointer, > fbdev_output::device will contain a stale pointer. > > Could you send a preparation patch that makes fbdev_output_create() use > strdup() for the device string, please? > > I think it might have leaked the device string allocated by > parse_options() before your patch. > >> if (b == NULL) >> return -1; >> return 0; >> diff --git a/src/compositor-fbdev.h b/src/compositor-fbdev.h >> new file mode 100644 >> index 0000000..48bd269 >> --- /dev/null >> +++ b/src/compositor-fbdev.h >> @@ -0,0 +1,49 @@ >> +/* >> + * 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 WESTON_COMPOSITOR_FBDEV_H >> +#define WESTON_COMPOSITOR_FBDEV_H >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include "compositor.h" >> + >> +#define WESTON_FBDEV_BACKEND_CONFIG_VERSION 1 >> + >> +struct weston_fbdev_backend_config { >> + struct weston_backend_config base; >> + >> + int tty; >> + char *device; >> + int use_gl; >> +}; >> + >> +#ifdef __cplusplus >> +} >> +#endif >> + >> +#endif /* WESTON_COMPOSITOR_FBDEV_H */ >> diff --git a/src/main.c b/src/main.c >> index fc97dd8..7257059 100644 >> --- a/src/main.c >> +++ b/src/main.c >> @@ -48,6 +48,7 @@ >> #include "version.h" >> >> #include "compositor-headless.h" >> +#include "compositor-fbdev.h" >> >> static struct wl_list child_process_list; >> static struct weston_compositor *segv_compositor; >> @@ -718,11 +719,41 @@ load_headless_backend(struct weston_compositor *c, >> char const * backend, >> } >> >> static int >> +load_fbdev_backend(struct weston_compositor *c, char const * backend, >> + int *argc, char **argv, struct weston_config *wc) >> +{ >> + struct weston_fbdev_backend_config config = {{ 0, }}; >> + int ret = 0; >> + >> + const struct weston_option fbdev_options[] = { >> + { WESTON_OPTION_INTEGER, "tty", 0, &config.tty }, >> + { WESTON_OPTION_STRING, "device", 0, &config.device }, >> + { WESTON_OPTION_BOOLEAN, "use-gl", 0, &config.use_gl }, >> + }; >> + >> + parse_options(fbdev_options, ARRAY_LENGTH(fbdev_options), argc, argv); > > Looks like you are missing to set the defaults for the command line > options. They are all just zero/NULL here. > > Note, that the config_init_to_defaults() in compositor-fbdev.c applies > only to the fields that main.c is not setting, but currently as this is > the first revision of weston_fbdev_backend_config, all fields are set by > main.c always. > > Did this run at all? > >> + >> + if(!config.device) > > Use a space after a keyword. > >> + config.device = strdup("/dev/fb0"); >> + >> + config.base.struct_version = WESTON_FBDEV_BACKEND_CONFIG_VERSION; >> + config.base.struct_size = sizeof(struct weston_fbdev_backend_config); >> + >> + /* load the actual wayland backend and configure it */ >> + ret = load_backend_new(c, backend, &config.base); >> + >> + free(config.device); >> + return ret; >> +} >> + >> +static int >> load_backend(struct weston_compositor *compositor, const char *backend, >> int *argc, char **argv, struct weston_config *config) >> { >> if (strstr(backend, "headless-backend.so")) >> return load_headless_backend(compositor, backend, argc, argv, >> config); >> + else if (strstr(backend, "fbdev-backend.so")) >> + return load_fbdev_backend(compositor, backend, argc, argv, >> config); >> #if 0 >> else if (strstr(backend, "drm-backend.so")) >> return load_drm_backend(compositor, backend, argc, argv, >> config); >> @@ -730,8 +761,6 @@ load_backend(struct weston_compositor *compositor, const >> char *backend, >> 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")) >> - return load_fbdev_backend(compositor, backend, argc, argv, >> config); >> else if (strstr(backend, "rpi-backend.so")) >> return load_rpi_backend(compositor, backend, argc, argv, >> config); >> else if (strstr(backend, "rdp-backend.so")) > > Otherwise this looks fine. > > > Thanks, > pq > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel