[2/2] libio: Update tst-fmemopen2.c

Reposting to see if I can land it for 2.22.

--

This patch updates tst-fmemopen2 to check for fmemopen with NULL buffer
inputs and also refactor the code a bit.

The test relies on a POSIX compliant fmemopen implementation.

Tested on x86_64, i386, aarch64, and arm-linux-gnueabihf.

	* stdio-common/tst-fmemopen2.c (do_test): Add test for NULL and zero
	length buffers.
	* stdio-common/tst-fmemopen.c (do_test): Refactor to use
	test-skeleton.c.

--

On 06/16/2015 09:56 AM, Adhemerval Zanella wrote:
> Reposting to see if I can land it for 2.22.
> 
> --
> 
> This patch updates tst-fmemopen2 to check for fmemopen with NULL buffer
> inputs and also refactor the code a bit.
> 
> The test relies on a POSIX compliant fmemopen implementation.
> 
> Tested on x86_64, i386, aarch64, and arm-linux-gnueabihf.
> 
> 	* stdio-common/tst-fmemopen2.c (do_test): Add test for NULL and zero
> 	length buffers.
> 	* stdio-common/tst-fmemopen.c (do_test): Refactor to use
> 	test-skeleton.c.

OK with nits fixed and after you checkin the fmemopen fixes.

> --
> 
> diff --git a/stdio-common/tst-fmemopen2.c b/stdio-common/tst-fmemopen2.c
> index e9d8b63..3c9dc98 100644
> --- a/stdio-common/tst-fmemopen2.c
> +++ b/stdio-common/tst-fmemopen2.c
> @@ -1,71 +1,253 @@
> +/* fmemopen tests.
> +   Copyright (C) 2014-2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +

OK.

>  #include <assert.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <sys/types.h>
> -
> +#include <errno.h>
>  

Please add a comment explaining what the do_test_with_buffer function does.

>  static int
> -do_test (void)
> +do_test_with_buffer (void)
>  {
>    int result = 0;
>    char buf[100];
> -  FILE *fp = fmemopen (buf, sizeof (buf), "w");
> +  const size_t nbuf = sizeof (buf);
> +
> +  FILE *fp = fmemopen (buf, nbuf, "w");
>    if (fp == NULL)
>      {
> -      puts ("fmemopen failed");
> -      return 0;
> +      printf ("%s: fmemopen failed\n", __FUNCTION__);

We should use "FAIL:" or "PASS:" text here in the prefix so
we can start tracking all the places we have multiple tests
in a single test (and eventually split out the reporting).

> +      return 1;
>      }
> +
>    static const char str[] = "hello world";
> -#define nstr (sizeof (str) - 1)
> +  const size_t nstr = sizeof (str) - 1;
>    fputs (str, fp);
>    off_t o = ftello (fp);
>    if (o != nstr)
>      {
> -      printf ("first ftello returned %jd, expected %zu\n",
> -	      (intmax_t) o, nstr);
> +      printf ("%s: first ftello returned %jd, expected %zu\n",
> +	      __FUNCTION__, (intmax_t)o, nstr);
>        result = 1;
>      }
> +
>    rewind (fp);
>    o = ftello (fp);
>    if (o != 0)
>      {
> -      printf ("second ftello returned %jd, expected 0\n", (intmax_t) o);
> +      printf ("%s: second ftello returned %jd, expected 0\n",
> +	      __FUNCTION__, (intmax_t)o);
>        result = 1;
>      }
>    if (fseeko (fp, 0, SEEK_END) != 0)
>      {
> -      puts ("fseeko failed");
> -      return 1;
> +      printf ("%s: fseeko failed\n", __FUNCTION__);
> +      result = 1;
>      }
>    o = ftello (fp);
>    if (o != nstr)
>      {
> -      printf ("third ftello returned %jd, expected %zu\n",
> -	      (intmax_t) o, nstr);
> +      printf ("%s: third ftello returned %jd, expected %zu\n",
> +	      __FUNCTION__, (intmax_t)o, nbuf);
>        result = 1;
>      }
> +
>    rewind (fp);
>    static const char str2[] = "just hello";
> -#define nstr2 (sizeof (str2) - 1)
> +  const size_t nstr2 = sizeof (str2) - 1;
>    assert (nstr2 < nstr);
>    fputs (str2, fp);
>    o = ftello (fp);
>    if (o != nstr2)
>      {
> -      printf ("fourth ftello returned %jd, expected %zu\n",
> -	      (intmax_t) o, nstr2);
> +      printf ("%s: fourth ftello returned %jd, expected %zu\n",
> +	      __FUNCTION__, (intmax_t)o, nstr2);
>        result = 1;
>      }
>    fclose (fp);
> +
>    static const char str3[] = "just hellod";
>    if (strcmp (buf, str3) != 0)
>      {
> -      printf ("final string is \"%s\", expected \"%s\"\n",
> -              buf, str3);
> +      printf ("%s: final string is \"%s\", expected \"%s\"\n",
> +              __FUNCTION__, buf, str3);
> +      result = 1;
> +    }
> +  return result;
> +}
> +

The do_test_without_buffer also needs a comment explaining briefly
what it does.

> +static int
> +do_test_without_buffer (void)
> +{
> +  int result = 0;
> +  const size_t nbuf = 100;
> +
> +  FILE *fp = fmemopen (NULL, nbuf, "w");
> +  if (fp == NULL)
> +    {
> +      printf ("%s: fmemopen failed\n", __FUNCTION__);
> +      return 1;
> +    }
> +
> +  static const char str[] = "hello world";
> +  const size_t nstr = sizeof (str) - 1;
> +
> +  fputs (str, fp);
> +  off_t o = ftello (fp);
> +  if (o != nstr)
> +    {
> +      printf ("%s: first ftello returned %ld, expected %zu\n",
> +	      __FUNCTION__, o, nstr);
> +      result = 1;
> +    }
> +  if (fseeko (fp, 0, SEEK_END) != 0)
> +    {
> +      printf ("%s: fseeko failed\n", __FUNCTION__);
> +      result = 1;
> +    }
> +  o = ftello (fp);
> +  if (o != nstr)
> +    {
> +      printf ("%s: second ftello returned %ld, expected %zu\n",
> +	      __FUNCTION__, o, nbuf);
>        result = 1;
>      }
> +  rewind (fp);
> +  static const char str2[] = "just hello";
> +  const size_t nstr2 = sizeof (str2) - 1;
> +  assert (nstr2 < nstr);
> +  fputs (str2, fp);
> +  o = ftello (fp);
> +  if (o != nstr2)
> +    {
> +      printf ("%s: third ftello returned %ld, expected %zu\n",
> +	      __FUNCTION__, o, nstr2);
> +      result = 1;
> +    }
> +  fclose (fp);
> +
>    return result;
>  }
>  

Likewise.

> +static int
> +do_test_length_zero (void)
> +{
> +  int result = 0;
> +  FILE *fp;
> +#define BUFCONTENTS "testing buffer"
> +  char buf[100] = BUFCONTENTS;
> +  const size_t nbuf = 0;
> +  int r;
> +
> +  fp = fmemopen (buf, nbuf, "r");
> +  if (fp == NULL)
> +    {
> +      printf ("%s: fmemopen failed\n", __FUNCTION__);
> +      return 1;
> +    }
> +
> +  /* Reading any data on a zero-length buffer should return EOF.  */
> +  if ((r = fgetc (fp)) != EOF)
> +    {
> +      printf ("%s: fgetc on a zero-length returned: %d\n",
> +	      __FUNCTION__, r);
> +      result = 1;
> +    }
> +  off_t o = ftello (fp);
> +  if (o != 0)
> +    {
> +      printf ("%s: first ftello returned %ld, expected 0\n",
> +	      __FUNCTION__, o);
> +      result = 1;
> +    }
> +  fclose (fp);
> +
> +  /* Writing any data shall start at current position and shall not pass
> +     current buffer size beyond the size in fmemopen call.  */
> +  fp = fmemopen (buf, nbuf, "w");
> +  if (fp == NULL)
> +    {
> +      printf ("%s: second fmemopen failed\n", __FUNCTION__);
> +      return 1;
> +    }
> +
> +  static const char str[] = "hello world";
> +  /* Because of buffering, fputs call itself don't fail, however the final

s/fputs call itself don't fail/the fputs call itself will not fail/g

> +     buffer should be not changed because of length 0 passed in fmemopen

s/of length 0 passed in fmemopen/length 0 was passed to the fmemopen/g

> +     call.  */
> +  fputs (str, fp);
> +  r = 0;
> +  errno = 0;
> +  if (fflush (fp) != EOF)
> +    {
> +      printf ("%s: fflush did not return EOF\n", __FUNCTION__);
> +      fclose (fp);
> +      return 1;
> +    }
> +  if (errno != ENOSPC)
> +    {
> +      printf ("%s: errno is %i (expected %i - ENOSPC)\n", __FUNCTION__,
> +	      errno, ENOSPC);
> +      fclose (fp);
> +      return 1;
> +    }
> +
> +  fclose (fp);
> +
> +  if (strcmp (buf, BUFCONTENTS) != 0)
> +    {
> +      printf ("%s: strcmp (%s, %s) failed\n", __FUNCTION__, buf,
> +	      BUFCONTENTS);
> +      return 1;
> +    }
> +
> +  /* Different than 'w' mode, 'w+' truncates the buffer.  */
> +  fp = fmemopen (buf, nbuf, "w+");
> +  if (fp == NULL)
> +    {
> +      printf ("%s: second fmemopen failed\n", __FUNCTION__);
> +      return 1;
> +    }
> +
> +  fclose (fp);
> +
> +  if (strcmp (buf, "") != 0)
> +    {
> +      printf ("%s: strcmp (%s, \"\") failed\n", __FUNCTION__, buf);
> +      return 1;
> +    }
> +
> +  return result;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int ret = 0;
> +
> +  ret += do_test_with_buffer ();
> +  ret += do_test_without_buffer ();
> +  ret += do_test_length_zero ();
> +
> +  return ret;
> +}
> +
>  #define TEST_FUNCTION do_test ()
>  #include "../test-skeleton.c"
> 

Cheers,
Carlos.

On 07-07-2015 16:48, Carlos O'Donell wrote:
> On 06/16/2015 09:56 AM, Adhemerval Zanella wrote:
>> Reposting to see if I can land it for 2.22.
>>
>> --
>>
>> This patch updates tst-fmemopen2 to check for fmemopen with NULL buffer
>> inputs and also refactor the code a bit.
>>
>> The test relies on a POSIX compliant fmemopen implementation.
>>
>> Tested on x86_64, i386, aarch64, and arm-linux-gnueabihf.
>>
>> 	* stdio-common/tst-fmemopen2.c (do_test): Add test for NULL and zero
>> 	length buffers.
>> 	* stdio-common/tst-fmemopen.c (do_test): Refactor to use
>> 	test-skeleton.c.
> 
> OK with nits fixed and after you checkin the fmemopen fixes.

Thanks for the review, I have updated the patch with all your requests and 
I have also  added some more comments about the tests intentions.

Hi,

i get the following failure for this test on s390-32:
FAIL: third ftello returned 28, expected 100
FAIL: final string is "just 
hellod`<80>@^]<92>Ua^C<B0>^?<9E>u<F8>^?<9E>u`", expected "just hellod"

I think the "expected 100" should be "expected 11". See testcode - there 
is nbuf instead of nstr as argument to printf:
   if (o != nstr)
     {
       printf ("FAIL: third ftello returned %jd, expected %zu\n",
	      (intmax_t)o, nbuf);
       result = 1;
     }

But the return value isn't 11 as tested by tst-fmemopen2.c.
If the testcase is linked against old-fmemopen, this test passes.
The problem is, that buf contains random characters.
fmemopen performs c->maxpos = strnlen (c->buffer, len);
Thus maxpos depends on the random characters in buf.

If the current position is set to the end of buffer with:
fseeko (fp, 0, SEEK_END)
, then ftello (fp); returns the value of "c->maxpos".

But according to 
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fmemopen.html:
"The stream shall also maintain the size of the current buffer contents; 
use of fseek() or fseeko() on the stream with SEEK_END shall seek 
relative to this size. For modes r and r+ the size shall be set to the 
value given by the size argument. For modes w and w+ the initial size 
shall be zero and for modes a and a+ the initial size shall be either 
the position of the first null byte in the buffer or the value of the 
size argument if no null byte is found.",

the initial size of the current buffer should be 0 for modes w and w+ 
instead of the first null byte in buffer.
For w+, fmemopen sets the fist character to zero before strnlen is called:
       if (mode[0] == 'w' && mode[1] == '+')
	c->buffer[0] = '\0';


The old-fmemopen function sets the first character to zero for w and w+:
       if (mode[0] == 'w')
	c->buffer[0] = '\0';


The test passes on s390-32 if the first character (or at a lower index 
than 11) is set to zero just before the fmemopen call.

If buf contains a string larger than "hello world", the testcase also 
fails on s390-64/x86_64.
Failure:
FAIL: third ftello returned 20, expected 100
FAIL: final string is "just hellod234567890", expected "just hellod"
if buf contains the following string before the fmemopen call:
   strcpy (buf, "12345678901234567890");

   FILE *fp = fmemopen (buf, nbuf, "w");


Is this is a bug in the new fmemopen implementation?!
If it isn't, the testcase should initialize buf with zeros.

Bye
Stefan

On 07/07/2015 11:23 PM, Adhemerval Zanella wrote:
>
>
> On 07-07-2015 16:48, Carlos O'Donell wrote:
>> On 06/16/2015 09:56 AM, Adhemerval Zanella wrote:
>>> Reposting to see if I can land it for 2.22.
>>>
>>> --
>>>
>>> This patch updates tst-fmemopen2 to check for fmemopen with NULL buffer
>>> inputs and also refactor the code a bit.
>>>
>>> The test relies on a POSIX compliant fmemopen implementation.
>>>
>>> Tested on x86_64, i386, aarch64, and arm-linux-gnueabihf.
>>>
>>> 	* stdio-common/tst-fmemopen2.c (do_test): Add test for NULL and zero
>>> 	length buffers.
>>> 	* stdio-common/tst-fmemopen.c (do_test): Refactor to use
>>> 	test-skeleton.c.
>>
>> OK with nits fixed and after you checkin the fmemopen fixes.
>
> Thanks for the review, I have updated the patch with all your requests and
> I have also  added some more comments about the tests intentions.
>