On Tue, 2 Sep 2014 00:14:45 +0900
Ryo Munakata <ryomnk...@gmail.com> wrote:
> Also some refactoring to achieve this.
>
> Signed-off-by: Ryo Munakata <ryomnk...@gmail.com>
> ---
> src/compositor.c | 52 +++++++++++++++++++++++++++-------------------------
> 1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/src/compositor.c b/src/compositor.c
> index 8705950..6adef94 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -4292,9 +4292,7 @@ int main(int argc, char *argv[])
> struct weston_config *config);
> int i, fd;
> char *backend = NULL;
> - char *option_backend = NULL;
> char *shell = NULL;
> - char *option_shell = NULL;
> char *modules, *option_modules = NULL;
> char *log = NULL;
> char *server_socket = NULL, *end;
> @@ -4309,8 +4307,8 @@ int main(int argc, char *argv[])
> struct wl_listener primary_client_destroyed;
>
> const struct weston_option core_options[] = {
> - { WESTON_OPTION_STRING, "backend", 'B', &option_backend },
> - { WESTON_OPTION_STRING, "shell", 0, &option_shell },
> + { WESTON_OPTION_STRING, "backend", 'B', &backend },
> + { WESTON_OPTION_STRING, "shell", 0, &shell },
This is a good simplification, getting rid of the option_* variables
and taking advantage of both option and config parsers to return
freshly allocated strings.
> { WESTON_OPTION_STRING, "socket", 'S', &socket_name },
> { WESTON_OPTION_INTEGER, "idle-time", 'i', &idle_time },
> { WESTON_OPTION_STRING, "modules", 0, &option_modules },
> @@ -4331,6 +4329,7 @@ int main(int argc, char *argv[])
> }
>
> weston_log_file_open(log);
> + free(log);
Nice!
>
> weston_log("%s\n"
> STAMP_SPACE "%s\n"
> @@ -4371,19 +4370,17 @@ int main(int argc, char *argv[])
> }
> section = weston_config_get_section(config, "core", NULL, NULL);
>
> - if (option_backend)
> - backend = strdup(option_backend);
> - else
> - weston_config_section_get_string(section, "backend", &backend,
> - NULL);
>
> if (!backend) {
> - if (getenv("WAYLAND_DISPLAY") || getenv("WAYLAND_SOCKET"))
> - backend = strdup("wayland-backend.so");
> - else if (getenv("DISPLAY"))
> - backend = strdup("x11-backend.so");
> - else
> - backend = strdup(WESTON_NATIVE_BACKEND);
> + weston_config_section_get_string(section, "backend", &backend,
> NULL);
The I like the logic up to this.
> + if (!backend) {
> + if (getenv("WAYLAND_DISPLAY") ||
> getenv("WAYLAND_SOCKET"))
> + backend = strdup("wayland-backend.so");
> + else if (getenv("DISPLAY"))
> + backend = strdup("x11-backend.so");
> + else
> + backend = strdup(WESTON_NATIVE_BACKEND);
> + }
While working here, I think this piece of code could be extracted to a
new function weston_choose_default_backend(). The extraction could be
the first patch in the series.
We might want to make the default backend choosing a bit smarter in the
future, like not attempting to use backends we did not build.
> }
>
> backend_init = weston_load_module(backend, "backend_init");
> @@ -4440,24 +4437,26 @@ int main(int argc, char *argv[])
> if (socket_name) {
> if (wl_display_add_socket(display, socket_name)) {
> weston_log("fatal: failed to add socket: %m\n");
> - ret = EXIT_FAILURE;
> - goto out;
> + free(socket_name);
> + socket_name = NULL;
> }
> } else {
> socket_name = wl_display_add_socket_auto(display);
> - if (!socket_name) {
> + if (socket_name)
> + socket_name = strdup(socket_name);
> + else
> weston_log("fatal: failed to add socket: %m\n");
> - ret = EXIT_FAILURE;
> - goto out;
> - }
> }
I think we could extract another function here, that would include
everything inside the else-clause of if (fd != -1), and call
it e.g. int weston_create_listening_socket(display, socket_name).
No need to return the final socket_name to main(), I believe.
Then in main() you could just unconditionally call free(socket_name)
once, since free(NULL) is ok.
>
> + if (!socket_name) {
> + ret = EXIT_FAILURE;
> + goto out;
> + }
> setenv("WAYLAND_DISPLAY", socket_name, 1);
> + free(socket_name);
> }
>
> - if (option_shell)
> - shell = strdup(option_shell);
> - else
> + if (!shell)
> weston_config_section_get_string(section, "shell", &shell,
> "desktop-shell.so");
>
> @@ -4474,8 +4473,11 @@ int main(int argc, char *argv[])
> }
> free(modules);
>
> - if (load_modules(ec, option_modules, &argc, argv) < 0)
> + if (load_modules(ec, option_modules, &argc, argv) < 0) {
> + free(option_modules);
> goto out;
> + }
> + free(option_modules);
>
> weston_compositor_wake(ec);
>
You could just have one free(option_modules) somewhere after out: label.
We could collect all free()s for the WESTON_OPTION_STRING variables at
the end of main().
Thanks,
pq
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
