[PATCH weston] compositor.c: do not leak option strings

[Pekka Paalanen]

On Tue,  2 Sep 2014 00:14:45 +0900
Ryo Munakata <ryomnktml at gmail.com> wrote:

> Also some refactoring to achieve this.
> 
> Signed-off-by: Ryo Munakata <ryomnktml at 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