[PATCH libinput] Handle errors that were previously being ignored.

Peter Hutterer peter.hutterer at who-t.net
Sun May 31 17:02:44 PDT 2015


fwiw, I prefer more expressive subject lines, makes it easier to immediately
identify commits in the log. specifically always include the 'topic' of the
patch target, e.g. test, tools, touchpad, timer, etc. so in your case:

  test: handle getcwd() and system() return values in litest

or something along those lines.

On Fri, May 29, 2015 at 07:10:10AM -0700, Jon A. Cruz wrote:
> Added code to check for errors in getcwd() and system() that
> were previously ignored and silently dropped.
> 
> Signed-off-by: Jon A. Cruz <jonc at osg.samsung.com>
> ---
>  test/litest.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/test/litest.c b/test/litest.c
> index 349eca0..e658c32 100644
> --- a/test/litest.c
> +++ b/test/litest.c
> @@ -85,7 +85,11 @@ litest_backtrace_get_lineno(const char *executable,
>  	unsigned int i;
>  
>  	if (!cwd[0])
> -		getcwd(cwd, sizeof(cwd));
> +		if (getcwd(cwd, sizeof(cwd)) == NULL) {
> +			cwd[0] = 0; /* contents otherwise undefined. */
> +			litest_log("getcwd failed: (%s)\n", strerror(errno));
> +			return false;
> +		}

coding style is to use a {} for anything more than a line, so you need a {
after if (!cwd[0]) (and around this condition).
looking at the rest of this I don't think we need to bother with an error
message, much less to with an early return. if cwd fails (unlikely enough)
just reset it to an empty string. the only effect this has is that the
backtrace has absolute filenames - no big deal either way.

>  
>  	sprintf (buffer,
>  		 ADDR2LINE " -C -e %s -i %lx",
> @@ -375,7 +379,15 @@ static struct list all_tests;
>  static void
>  litest_reload_udev_rules(void)
>  {
> -	system("udevadm control --reload-rules");
> +	int ret = system("udevadm control --reload-rules");
> +	if (ret == -1) {
> +		litest_log("Failed to execute: udevadm\n");
> +	} else if (WIFEXITED(ret)) {
> +		if (WEXITSTATUS(ret))
> +			litest_log("udevadm failed with %d", WEXITSTATUS(ret));
> +	} else if (WIFSIGNALED(ret)) {
> +		litest_log("udevadm terminated with signal %d", WTERMSIG(ret));
> +	}
>  }

use a litest_abort here please. since a failed udev rule almost certainly
causes the test to fail having a backtrace on which device it fails on would
be useful.

Cheers,
   Peter


More information about the wayland-devel mailing list