[weston] weston-terminal: Add a few nice features

[Bryce Harrington]

On Mon, Sep 21, 2015 at 03:24:49PM +0300, ahmet acar wrote:
> Hi.This trivial patch adds 'cwd' (current working directory)
> and 'command' (run specified command immediatelly after open)
> features to weston-terminal.So this adds more use cases
> and flexibility to our terminal.

Sounds useful!  I would suggestion though to make your commit message
more descriptive oriented, e.g. like:

  Subject:  [PATCH weston] weston-terminal: Add cwd and command options

  Add 'cwd' (current working directory) and 'command' (run specified
  command immediately after startop) to weston-terminal.

I'm going to apologize upfront, since what lies below is a pretty
intense review.  Please don't let this discourage you - much of the
comments are Wayland-specifics that as a new wayland contributor you've
just not picked up yet.  I hope you'll fix up your patch and resubmit,
because I think this will be a useful feature.

> Changes from:
> 
> commit 90e2d07ec1e6e46136e7b43f427a838f3e5b01ed
> Author: Bryce Harrington <bryce at osg.samsung.com>
> Date:   Thu Sep 17 16:33:48 2015 -0700

I'm not following how that commit relates to this patch?
Unless I'm missing something, I think you can omit mention of this in
your changelog entry.
 
> ---
>  clients/terminal.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/clients/terminal.c b/clients/terminal.c
> index c5d5d60..f4e70e8 100644
> --- a/clients/terminal.c
> +++ b/clients/terminal.c
> @@ -52,6 +52,8 @@ static char *option_font;
>  static int option_font_size;
>  static char *option_term;
>  static char *option_shell;
> +static char *option_cwd;
> +static char *option_command = "";

Don't set option_command to an empty string here, just leave it
undefined like the others, and let weston_config_section_get_string()
initialize it.

>  static struct wl_list terminal_list;
>  
> @@ -3046,6 +3048,19 @@ terminal_run(struct terminal *terminal, const char *path)
>  		window_set_fullscreen(terminal->window, 1);
>  	else
>  		terminal_resize(terminal, 80, 24);
> +		
> +	int cmdlen = strlen(option_command);

To keep to stricter C requirements, Wayland style prefers declaring
variables at the top of the function.  Although see next comment.

> +	
> +	if(cmdlen)

Wayland coding style has a space after the 'if' here.

But, I *think* option_command will be NULL if there's no value, in which
case you can eliminate the cmdlen variable and just check option_command
directly.  Then in the write() call below, in place of cmdlen you can
call strlen() inline there.

> +	{
> +	    char *newline = "\n";	
> +	    strcat(option_command,newline);

Unfortunately, I think this isn't going to work.  option_command is
created in weston_config_section_get_string() by doing a strdup, which I
believe creates a buffer sized exactly to the length of the string, no
more.  So, appending one more character on the end probably causes a
buffer overrun.  I think this would cause a crash.

In general, I'd recommend avoiding use of strcat and prefer strncat; if
you don't can't specify what the 'n' should be for that call, it should
be a signal to doublecheck your dest buffer allocation size.

But instead, I'd suggest using snprintf here as a simpler and safer
solution.

(Yes, string operations in C are a royal PITA.)

Also, btw, coding style requires spaces after commas in function calls.

> +	    if(write(master, option_command, cmdlen+1) == -1)
> +	    {
> +	        printf("An error occurred in %s at line %d\n", __FILE__, __LINE__);

Looks like in this code file, errors are printed using
"fprintf(stderr, ...)" so probably best to do similarly here.

It's probably unnecessary to print the file and line number here,
although it certainly doesn't hurt.  But you might give a more
descriptive error message.  "An error occurred" is kind of generic.  ;-)

Also, for code style, Wayland style would omit the brackets in an if
statement when it contains only a single statement.  So if you're just
doing the print, lose the brackets.  HOWEVER, you might want to consider
if some additional error handling should be done here (should the
terminal still be started up if the command fails, or perhaps should it
be terminated with an error code?)  You could probably work on this bit
in a followup patch if you'd prefer.

> +	    }
> +	}
>  
>  	return 0;
>  }
> @@ -3055,6 +3070,9 @@ static const struct weston_option terminal_options[] = {
>  	{ WESTON_OPTION_STRING, "font", 0, &option_font },
>  	{ WESTON_OPTION_INTEGER, "font-size", 0, &option_font_size },
>  	{ WESTON_OPTION_STRING, "shell", 0, &option_shell },
> +     { WESTON_OPTION_STRING, "cwd", 0, &option_cwd },
> +     { WESTON_OPTION_STRING, "command", 0, &option_command },
> +
>  };
>  
>  int main(int argc, char *argv[])
> @@ -3087,7 +3105,9 @@ int main(int argc, char *argv[])
>  		       "  --fullscreen or -f\n"
>  		       "  --font=NAME\n"
>  		       "  --font-size=SIZE\n"
> -		       "  --shell=NAME\n", argv[0]);
> +		       "  --shell=NAME\n"
> +		       "  --cwd=PATH\n"
> +                    "  --command=CMDLINE\n", argv[0]);
>  		return 1;
>  	}
>  
> @@ -3099,10 +3119,14 @@ int main(int argc, char *argv[])
>  
>  	wl_list_init(&terminal_list);
>  	terminal = terminal_create(d);
> -	if (terminal_run(terminal, option_shell))
> -		exit(EXIT_FAILURE);
>  
> +	if(chdir(option_cwd)){} //just for to suppress 'ignored return value' warning for happy gcc.	

Stylistically this would be better expanded out:

	if (chdir(option_cwd)) {
		// suppress 'ignored return value' warning for happy gcc.	
	}

However, you should probably think more on what the correct behavior
should be if we can't cd to the requested directory.  Maybe we should
bail at that point?

> +	if (terminal_run(terminal, option_shell))
> +		exit(EXIT_FAILURE);	
> +	
>  	display_run(d);
>  
>  	return 0;
>  }

Final comment, of a more general nature.  Whenever one adds execution of
user-supplied data to a program, it's worth contemplating security
implications.  While the weston clients are intended more for example
purposes than actual usability, this worry might be excessive, however
of any of the clients the terminal's most likely to get serious use.
Maybe other reviewers can give some comment on this.

Bryce