Message ID | 55802AFD.1040404@linaro.org |
---|---|
State | New |
Headers | show |
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. >
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/>. */ + + #include <assert.h> #include <stdio.h> #include <string.h> #include <sys/types.h> - +#include <errno.h> 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__); + 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; +} + +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; } +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 + buffer should be not changed because of length 0 passed in fmemopen + 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"