On Tue, 26 Apr 2016 21:10:55 +0200 Benoit Gschwind <gschw...@gnu-log.net> wrote:
> Implement a "well" defined API to configure the fbdev backend.
> Following and according to discution about libweston API
> ---
> Signed-off-by: Benoit Gschwind <gschw...@gnu-log.net>
This S-o-b is on the wrong side of the '---'. git-am will drop if it is
below the '---'.
>
> v1:
> - add src/compositor-fbdev.h in the Makefile.am
> - fix typo
>
> Makefile.am | 3 ++-
> src/compositor-fbdev.c | 45 +++++++++++++++++++++++----------------------
> src/compositor-fbdev.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/main.c | 33 +++++++++++++++++++++++++++++++--
> 4 files changed, 105 insertions(+), 25 deletions(-)
> create mode 100644 src/compositor-fbdev.h
Hi Benoit,
would have been nice to rebase this on upstream master branch, but I
handled the conflicts for now to review it.
>
> diff --git a/Makefile.am b/Makefile.am
> index c042c68..d319786 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -72,7 +72,8 @@ weston_SOURCES = \
> src/log.c \
> src/compositor.c \
> src/compositor.h \
> - src/compositor-headless.h \
> + src/compositor-headless.h \
> + src/compositor-fbdev.h \
> src/input.c \
> src/data-device.c \
> src/screenshooter.c \
Makefile.am is still missing the other two places to add
compositor-fbdev.h to.
> 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);
I am quoting the exact same comment I gave the last time:
> 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.
What we talked about in IRC was the other comment, not this one.
Please, look again at the call chain after applying your patch:
main.c:
- load_fbdev_backend():
The config.device string is malloc'd, and passed in to
load_backend_new().
compositor-fbdev.c:
- backend_init(): Just copies the pointer, does not reallocate the string.
- fbdev_backend_create(): Calls fbdev_output_create().
- fbdev_output_create(): stores the string pointer in struct
fbdev_output::device *without* copying.
main.c:
- load_fbdev_backend(): calls free() on the pointer.
This results in struct fbdev_output::device containing a pointer to
freed memory.
> if (b == NULL)
> return -1;
> return 0;
Thanks,
pq
pgpDxgXEE0zq_.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
