Re: [PATCH] test-skeleton.c: Use stdout for error messages

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Fri, Apr 11, 2014 at 03:49:29PM +0100, Will Newton wrote:
> At the moment the test skeleton uses a mixture of stdout and
> stderr for error message output. Using stdout for all test output
> keeps all output correctly ordered and properly redirected to the
> output file. The suggestion to use stdout is also made on the wiki:
> 
> https://sourceware.org/glibc/wiki/Testing/Testsuite#Writing_a_test_case
> 
> ChangeLog:
> 
> 2014-04-11  Will Newton  <will.newton@linaro.org>
> 
> 	* test-skeleton.c (signal_handler): Use printf and strerror
> 	rather than perror.  Use printf rather than fprintf to
> 	stderr.
> 	(main): Likewise.
> ---
>  test-skeleton.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/test-skeleton.c b/test-skeleton.c
> index d7d2f75..dd7de8b 100644
> --- a/test-skeleton.c
> +++ b/test-skeleton.c
> @@ -160,7 +160,7 @@ signal_handler (int sig __attribute__ ((unused)))
>      }
>    if (killed != 0 && killed != pid)
>      {
> -      perror ("Failed to kill test process");
> +      printf ("Failed to kill test process: %s\n", strerror (errno));

You could just use %m here.  Likewise for other instances of strerror.

>        exit (1);
>      }
>  
> @@ -181,16 +181,16 @@ signal_handler (int sig __attribute__ ((unused)))
>  #endif
>  
>    if (killed == 0 || (WIFSIGNALED (status) && WTERMSIG (status) == SIGKILL))
> -    fputs ("Timed out: killed the child process\n", stderr);
> +    puts ("Timed out: killed the child process\n");
>    else if (WIFSTOPPED (status))
> -    fprintf (stderr, "Timed out: the child process was %s\n",
> -	     strsignal (WSTOPSIG (status)));
> +    printf ("Timed out: the child process was %s\n",
> +	    strsignal (WSTOPSIG (status)));
>    else if (WIFSIGNALED (status))
> -    fprintf (stderr, "Timed out: the child process got signal %s\n",
> -	     strsignal (WTERMSIG (status)));
> +    printf ("Timed out: the child process got signal %s\n",
> +	    strsignal (WTERMSIG (status)));
>    else
> -    fprintf (stderr, "Timed out: killed the child process but it exited %d\n",
> -	     WEXITSTATUS (status));
> +    printf ("Timed out: killed the child process but it exited %d\n",
> +	    WEXITSTATUS (status));
>  
>    /* Exit with an error.  */
>    exit (1);
> @@ -275,7 +275,7 @@ main (int argc, char *argv[])
>  
>        if (chdir (test_dir) < 0)
>  	{
> -	  perror ("chdir");
> +	  printf ("chdir: %s\n", strerror (errno));
>  	  exit (1);
>  	}
>      }
> @@ -334,10 +334,10 @@ main (int argc, char *argv[])
>  	    data_limit.rlim_cur = MIN ((rlim_t) TEST_DATA_LIMIT,
>  				       data_limit.rlim_max);
>  	  if (setrlimit (RLIMIT_DATA, &data_limit) < 0)
> -	    perror ("setrlimit: RLIMIT_DATA");
> +	    printf ("setrlimit: RLIMIT_DATA: %s\n", strerror (errno));
>  	}
>        else
> -	perror ("getrlimit: RLIMIT_DATA");
> +	printf ("getrlimit: RLIMIT_DATA: %s\n", strerror (errno));
>  #endif
>  
>        /* We put the test process in its own pgrp so that if it bogusly
> @@ -349,7 +349,7 @@ main (int argc, char *argv[])
>      }
>    else if (pid < 0)
>      {
> -      perror ("Cannot fork test program");
> +      printf ("Cannot fork test program: %s\n", strerror (errno));
>        exit (1);
>      }
>  
> @@ -387,18 +387,16 @@ main (int argc, char *argv[])
>        if (EXPECTED_SIGNAL != 0)
>  	{
>  	  if (WTERMSIG (status) == 0)
> -	    fprintf (stderr,
> -		     "Expected signal '%s' from child, got none\n",
> -		     strsignal (EXPECTED_SIGNAL));
> +	    printf ("Expected signal '%s' from child, got none\n",
> +		    strsignal (EXPECTED_SIGNAL));
>  	  else
> -	    fprintf (stderr,
> -		     "Incorrect signal from child: got `%s', need `%s'\n",
> -		     strsignal (WTERMSIG (status)),
> -		     strsignal (EXPECTED_SIGNAL));
> +	    printf ("Incorrect signal from child: got `%s', need `%s'\n",
> +		    strsignal (WTERMSIG (status)),
> +		    strsignal (EXPECTED_SIGNAL));
>  	}
>        else
> -	fprintf (stderr, "Didn't expect signal from child: got `%s'\n",
> -		 strsignal (WTERMSIG (status)));
> +	printf ("Didn't expect signal from child: got `%s'\n",
> +		strsignal (WTERMSIG (status)));
>        exit (1);
>      }
>  
> @@ -408,8 +406,8 @@ main (int argc, char *argv[])
>  #else
>    if (WEXITSTATUS (status) != EXPECTED_STATUS)
>      {
> -      fprintf (stderr, "Expected status %d, got %d\n",
> -	       EXPECTED_STATUS, WEXITSTATUS (status));
> +      printf ("Expected status %d, got %d\n",
> +	      EXPECTED_STATUS, WEXITSTATUS (status));
>        exit (1);
>      }
>  

This looks good to me with the above changes, but I think a senior
maintainer should review this as well before it goes in.

Siddhesh

Attachment: pgp0iiJXsIIsI.pgp
Description: PGP signature