diff mbox

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

Message ID 55A671BE.6090306@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto July 15, 2015, 2:44 p.m. UTC
Hi Stefan,

Thanks for checking it.

On 15-07-2015 10:46, Stefan Liebler wrote:
> 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;
>     }
> 
> [...]
> 
> Is this is a bug in the new fmemopen implementation?!
> If it isn't, the testcase should initialize buf with zeros.

Yes, it is an issue with new implementation since it requires to open the stream
to first null byte (thus the strnlen) only for 'a' mode.  The follow patch should 
fix the behaviour you are seeing:

Carlos, if you allow I would like to push this modification for 2.22.

> 
> 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.
>>
>

Comments

Stefan Liebler July 15, 2015, 3:40 p.m. UTC | #1
The testcase is passing on s390-32/64 with these changes.

On 07/15/2015 04:44 PM, Adhemerval Zanella wrote:
> Hi Stefan,
>
> Thanks for checking it.
>
> On 15-07-2015 10:46, Stefan Liebler wrote:
>> 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;
>>      }
>>
>> [...]
>>
>> Is this is a bug in the new fmemopen implementation?!
>> If it isn't, the testcase should initialize buf with zeros.
>
> Yes, it is an issue with new implementation since it requires to open the stream
> to first null byte (thus the strnlen) only for 'a' mode.  The follow patch should
> fix the behaviour you are seeing:
>
> diff --git a/libio/fmemopen.c b/libio/fmemopen.c
> index e6e6a49..3ab3e8d 100644
> --- a/libio/fmemopen.c
> +++ b/libio/fmemopen.c
> @@ -150,7 +150,7 @@ __fmemopen (void *buf, size_t len, const char *mode)
>     cookie_io_functions_t iof;
>     fmemopen_cookie_t *c;
>
> -  c = (fmemopen_cookie_t *) malloc (sizeof (fmemopen_cookie_t));
> +  c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1);
>     if (c == NULL)
>       return NULL;
>
> @@ -165,7 +165,6 @@ __fmemopen (void *buf, size_t len, const char *mode)
>            return NULL;
>          }
>         c->buffer[0] = '\0';
> -      c->maxpos = 0;
>       }
>     else
>       {
> @@ -182,7 +181,8 @@ __fmemopen (void *buf, size_t len, const char *mode)
>         if (mode[0] == 'w' && mode[1] == '+')
>          c->buffer[0] = '\0';
>
> -      c->maxpos = strnlen (c->buffer, len);
> +      if (mode[0] == 'a')
> +        c->maxpos = strnlen (c->buffer, len);
>       }
>
>
> Carlos, if you allow I would like to push this modification for 2.22.
>
>>
>> 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.
>>>
>>
>
Carlos O'Donell July 16, 2015, 4:54 a.m. UTC | #2
> diff --git a/libio/fmemopen.c b/libio/fmemopen.c
> index e6e6a49..3ab3e8d 100644
> --- a/libio/fmemopen.c
> +++ b/libio/fmemopen.c
> @@ -150,7 +150,7 @@ __fmemopen (void *buf, size_t len, const char *mode)
>    cookie_io_functions_t iof;
>    fmemopen_cookie_t *c;
>  
> -  c = (fmemopen_cookie_t *) malloc (sizeof (fmemopen_cookie_t));
> +  c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1);
>    if (c == NULL)
>      return NULL;
>  
> @@ -165,7 +165,6 @@ __fmemopen (void *buf, size_t len, const char *mode)
>           return NULL;
>         }
>        c->buffer[0] = '\0';
> -      c->maxpos = 0;
>      }
>    else
>      {
> @@ -182,7 +181,8 @@ __fmemopen (void *buf, size_t len, const char *mode)
>        if (mode[0] == 'w' && mode[1] == '+')
>         c->buffer[0] = '\0';
>  
> -      c->maxpos = strnlen (c->buffer, len);
> +      if (mode[0] == 'a')
> +        c->maxpos = strnlen (c->buffer, len);
>      }
>  
>  
> Carlos, if you allow I would like to push this modification for 2.22.

Please post a full patch in a new thread with comments about what happened.

I'll ack that new thread.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/libio/fmemopen.c b/libio/fmemopen.c
index e6e6a49..3ab3e8d 100644
--- a/libio/fmemopen.c
+++ b/libio/fmemopen.c
@@ -150,7 +150,7 @@  __fmemopen (void *buf, size_t len, const char *mode)
   cookie_io_functions_t iof;
   fmemopen_cookie_t *c;
 
-  c = (fmemopen_cookie_t *) malloc (sizeof (fmemopen_cookie_t));
+  c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1);
   if (c == NULL)
     return NULL;
 
@@ -165,7 +165,6 @@  __fmemopen (void *buf, size_t len, const char *mode)
          return NULL;
        }
       c->buffer[0] = '\0';
-      c->maxpos = 0;
     }
   else
     {
@@ -182,7 +181,8 @@  __fmemopen (void *buf, size_t len, const char *mode)
       if (mode[0] == 'w' && mode[1] == '+')
        c->buffer[0] = '\0';
 
-      c->maxpos = strnlen (c->buffer, len);
+      if (mode[0] == 'a')
+        c->maxpos = strnlen (c->buffer, len);
     }