Fix up return codes for tests in tst-ftell-active-handler

Message ID 20140310121657.GE1656@spoyarek.pnq.redhat.com
State Committed
Headers

Commit Message

Siddhesh Poyarekar March 10, 2014, 12:16 p.m. UTC
  Hi,

The test functions used a variable ret to store failure codes for
individual tests, but the variable was incorrectly used to record
other failure codes too, resulting in overwriting of the tests status.
This is now fixed by making sure that the ret variable is used only
for recording test failures.

Siddhesh

	* libio/tst-ftell-active-handler.c (do_ftell_test): Don't mix
	up test status with function return status.
	(do_write_test): Likewise.
	(do_append_test): Likewise.

---
 libio/tst-ftell-active-handler.c | 49 +++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 18 deletions(-)
  

Comments

Carlos O'Donell March 14, 2014, 11:06 p.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2014 08:16 AM, Siddhesh Poyarekar wrote:
> Hi,
> 
> The test functions used a variable ret to store failure codes for
> individual tests, but the variable was incorrectly used to record
> other failure codes too, resulting in overwriting of the tests status.
> This is now fixed by making sure that the ret variable is used only
> for recording test failures.
> 
> Siddhesh
> 
> 	* libio/tst-ftell-active-handler.c (do_ftell_test): Don't mix
> 	up test status with function return status.
> 	(do_write_test): Likewise.
> 	(do_append_test): Likewise.

Looks good to me.

> ---
>  libio/tst-ftell-active-handler.c | 49 +++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
> index 54bfe63..5d5fc26 100644
> --- a/libio/tst-ftell-active-handler.c
> +++ b/libio/tst-ftell-active-handler.c
> @@ -119,17 +119,20 @@ do_ftell_test (const char *filename)
>  	{
>  	  FILE *fp;
>  	  int fd;
> +	  int fileret;
> +

OK.

>  	  printf ("\tftell: %s (file, \"%s\"): ", j == 0 ? "fdopen" : "fopen",
>  		  test_modes[i].mode);
>  
>  	  if (j == 0)
> -	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> -				      test_modes[i].mode);
> +	    fileret = get_handles_fdopen (filename, fd, fp,
> +					  test_modes[i].fd_mode,
> +					  test_modes[i].mode);

OK.

>  	  else
> -	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> +	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);

OK.

>  
> -	  if (ret != 0)
> -	    return ret;
> +	  if (fileret != 0)
> +	    return fileret;

OK.

So far I don't see any mixing of error returns.

If the error is non-zero then we return from do_ftell_test.

>  
>  	  long off = ftell (fp);
>  	  if (off != test_modes[i].old_off)
> @@ -143,7 +146,12 @@ do_ftell_test (const char *filename)
>  
>  	  /* The effect of this write on the offset should be seen in the ftell
>  	     call that follows it.  */
> -	  int ret = write (fd, data, data_len);
> +	  int write_ret = write (fd, data, data_len);

OK. Alright this is bad since it can overwrite ret set above e.g. ret |= 1;

> +	  if (write_ret != data_len)
> +	    {
> +	      printf ("write failed (%m)\n");
> +	      ret |= 1;
> +	    }

OK. Good check.

>  	  off = ftell (fp);
>  
>  	  if (off != test_modes[i].new_off)
> @@ -184,21 +192,23 @@ do_write_test (const char *filename)
>      {
>        for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
>  	{
> +	  int fileret;

OK.

>  	  printf ("\twrite: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
>  		  test_modes[i].mode);
>  
>  	  if (j == 0)
> -	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> +	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
>  	  else
> -	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> -				      test_modes[i].mode);
> +	    fileret = get_handles_fdopen (filename, fd, fp,
> +					  test_modes[i].fd_mode,
> +					  test_modes[i].mode);


OK.

>  
> -	  if (ret != 0)
> -	    return ret;
> +	  if (fileret != 0)
> +	    return fileret;

OK.

>  
>  	  /* Move offset to just before the end of the file.  */
> -	  off_t ret = lseek (fd, file_len - 1, SEEK_SET);
> -	  if (ret == -1)
> +	  off_t seek_ret = lseek (fd, file_len - 1, SEEK_SET);
> +	  if (seek_ret == -1)

OK.

>  	    {
>  	      printf ("lseek failed: %m\n");
>  	      ret |= 1;
> @@ -258,17 +268,20 @@ do_append_test (const char *filename)
>      {
>        for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
>  	{
> +	  int fileret;
> +

OK.

>  	  printf ("\tappend: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
>  		  test_modes[i].mode);
>  
>  	  if (j == 0)
> -	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> +	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);

OK.

>  	  else
> -	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> -				      test_modes[i].mode);
> +	    fileret = get_handles_fdopen (filename, fd, fp,
> +					  test_modes[i].fd_mode,
> +					  test_modes[i].mode);

OK.

>  
> -	  if (ret != 0)
> -	    return ret;
> +	  if (fileret != 0)
> +	    return fileret;

OK.

>  
>  	  /* Write some data.  */
>  	  size_t written = fputs_func (data, fp);
> 

Cheers,
Carlos.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTI4tjAAoJECXvCkNsKkr/p2EH/Rn5xYXK9hYrOrSAzSCi+P8c
wPxXmG+smAVTbmPk212j2w2/xZr2WWqYdr1eDBp/dHQYEpyDcbeTSNFj/dmppyvS
BxHCP4ALPDnRS19IXyrmRfDIHUVFvP8vR2OVoBZ2taMtl9YpfqL6g8uOYA3pJEal
d8jlfJhCQW34vCyFojsyx7yYGJcK6putoMUIe+g7DSnkiKH3RAMFyHL80ZKFPSXK
8XDbI0lHFNO/XWxebLIXLZjrQajDmgTdkc4ySucqqXLr9AjJGgYdZzOOyNZIb1Bz
2pSTOhrorqCJNtgu2FR0VX+Ll6/WhidhVIBdidhKWs6WsrVWqdcP1lcWzNHK11A=
=jGR0
-----END PGP SIGNATURE-----
  

Patch

diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
index 54bfe63..5d5fc26 100644
--- a/libio/tst-ftell-active-handler.c
+++ b/libio/tst-ftell-active-handler.c
@@ -119,17 +119,20 @@  do_ftell_test (const char *filename)
 	{
 	  FILE *fp;
 	  int fd;
+	  int fileret;
+
 	  printf ("\tftell: %s (file, \"%s\"): ", j == 0 ? "fdopen" : "fopen",
 		  test_modes[i].mode);
 
 	  if (j == 0)
-	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
-				      test_modes[i].mode);
+	    fileret = get_handles_fdopen (filename, fd, fp,
+					  test_modes[i].fd_mode,
+					  test_modes[i].mode);
 	  else
-	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
+	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
 
-	  if (ret != 0)
-	    return ret;
+	  if (fileret != 0)
+	    return fileret;
 
 	  long off = ftell (fp);
 	  if (off != test_modes[i].old_off)
@@ -143,7 +146,12 @@  do_ftell_test (const char *filename)
 
 	  /* The effect of this write on the offset should be seen in the ftell
 	     call that follows it.  */
-	  int ret = write (fd, data, data_len);
+	  int write_ret = write (fd, data, data_len);
+	  if (write_ret != data_len)
+	    {
+	      printf ("write failed (%m)\n");
+	      ret |= 1;
+	    }
 	  off = ftell (fp);
 
 	  if (off != test_modes[i].new_off)
@@ -184,21 +192,23 @@  do_write_test (const char *filename)
     {
       for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
 	{
+	  int fileret;
 	  printf ("\twrite: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
 		  test_modes[i].mode);
 
 	  if (j == 0)
-	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
+	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
 	  else
-	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
-				      test_modes[i].mode);
+	    fileret = get_handles_fdopen (filename, fd, fp,
+					  test_modes[i].fd_mode,
+					  test_modes[i].mode);
 
-	  if (ret != 0)
-	    return ret;
+	  if (fileret != 0)
+	    return fileret;
 
 	  /* Move offset to just before the end of the file.  */
-	  off_t ret = lseek (fd, file_len - 1, SEEK_SET);
-	  if (ret == -1)
+	  off_t seek_ret = lseek (fd, file_len - 1, SEEK_SET);
+	  if (seek_ret == -1)
 	    {
 	      printf ("lseek failed: %m\n");
 	      ret |= 1;
@@ -258,17 +268,20 @@  do_append_test (const char *filename)
     {
       for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
 	{
+	  int fileret;
+
 	  printf ("\tappend: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
 		  test_modes[i].mode);
 
 	  if (j == 0)
-	    ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
+	    fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
 	  else
-	    ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
-				      test_modes[i].mode);
+	    fileret = get_handles_fdopen (filename, fd, fp,
+					  test_modes[i].fd_mode,
+					  test_modes[i].mode);
 
-	  if (ret != 0)
-	    return ret;
+	  if (fileret != 0)
+	    return fileret;
 
 	  /* Write some data.  */
 	  size_t written = fputs_func (data, fp);