| Message ID | 20241106160355.2626947-1-fberat@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] libio: Add test case for fflush | expand |
* Frédéric Bérat: > I also tested the following: > char s[3] = ""; > fgets (s, 2, FILE); > ungetc (s[1], FILE); > fflush (FILE); > pos = lseek (SEEK_CUR); > > Which gives a `pos` value of 2. What's the file contents at this point, and where does fgets start reading? Thanks, Florian
On Wed, Nov 6, 2024 at 6:10 PM Florian Weimer <fweimer@redhat.com> wrote: > * Frédéric Bérat: > > > I also tested the following: > > char s[3] = ""; > > fgets (s, 2, FILE); > > ungetc (s[1], FILE); > > fflush (FILE); > > pos = lseek (SEEK_CUR); > > > > Which gives a `pos` value of 2. > > What's the file contents at this point, and where does fgets start > reading? > Unless I made a mistake somewhere, the file content is something like "0:1" (without the "), fgets starts to read at the beginning of the file (so does fgetc). > Thanks, > Florian > >
* Frederic Berat: > On Wed, Nov 6, 2024 at 6:10 PM Florian Weimer <fweimer@redhat.com> wrote: > > * Frédéric Bérat: > > > I also tested the following: > > char s[3] = ""; > > fgets (s, 2, FILE); > > ungetc (s[1], FILE); > > fflush (FILE); > > pos = lseek (SEEK_CUR); > > > > Which gives a `pos` value of 2. > > What's the file contents at this point, and where does fgets start > reading? > > Unless I made a mistake somewhere, the file content is something like > "0:1" (without the "), fgets starts to read at the beginning of the > file (so does fgetc). Sorry, the posted patch does not contain an fgets call. I'm not sure how I can help you debugging this if I can't even see the code. 8-) Can you post somewhere what you are doing? Thanks, Florian
On Thu, Nov 7, 2024 at 10:27 AM Florian Weimer <fweimer@redhat.com> wrote: > * Frederic Berat: > > > On Wed, Nov 6, 2024 at 6:10 PM Florian Weimer <fweimer@redhat.com> > wrote: > > > > * Frédéric Bérat: > > > > > I also tested the following: > > > char s[3] = ""; > > > fgets (s, 2, FILE); > > > ungetc (s[1], FILE); > > > fflush (FILE); > > > pos = lseek (SEEK_CUR); > > > > > > Which gives a `pos` value of 2. > > > > What's the file contents at this point, and where does fgets start > > reading? > > > > Unless I made a mistake somewhere, the file content is something like > > "0:1" (without the "), fgets starts to read at the beginning of the > > file (so does fgetc). > > Sorry, the posted patch does not contain an fgets call. I'm not sure > how I can help you debugging this if I can't even see the code. 8-) > > Can you post somewhere what you are doing? > Hopefully the attached file will do. $> make fflush_fgets $> ./fflush_fgets fflush_fgets.c Without unget: fgets: Position in stream before fflush: 1424 fgets: Position in stream after fflush: 1 With unget: fgets: Position in stream before fflush: 1424 fgets: Position in stream after fflush: 1423 Without unget: fgetc: Position in stream before fflush: 1424 fgetc: Position in stream after fflush: 2 With unget: fgetc: Position in stream before fflush: 1424 fgetc: Position in stream after fflush: 1 > Thanks, > Florian > >
* Frederic Berat: > On Thu, Nov 7, 2024 at 10:27 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Frederic Berat: > > > On Wed, Nov 6, 2024 at 6:10 PM Florian Weimer <fweimer@redhat.com> wrote: > > > > * Frédéric Bérat: > > > > > I also tested the following: > > > char s[3] = ""; > > > fgets (s, 2, FILE); > > > ungetc (s[1], FILE); > > > fflush (FILE); > > > pos = lseek (SEEK_CUR); > > > > > > Which gives a `pos` value of 2. > Without unget: > fgetc: Position in stream before fflush: 1424 > fgetc: Position in stream after fflush: 2 That's the only value 2 in the test, and it's using fgetc, not fgets? Thanks, Florian
On Thu, Nov 7, 2024 at 11:07 AM Florian Weimer <fweimer@redhat.com> wrote: > * Frederic Berat: > > > On Thu, Nov 7, 2024 at 10:27 AM Florian Weimer <fweimer@redhat.com> > wrote: > > > > * Frederic Berat: > > > > > On Wed, Nov 6, 2024 at 6:10 PM Florian Weimer <fweimer@redhat.com> > wrote: > > > > > > * Frédéric Bérat: > > > > > > > I also tested the following: > > > > char s[3] = ""; > > > > fgets (s, 2, FILE); > > > > ungetc (s[1], FILE); > > > > fflush (FILE); > > > > pos = lseek (SEEK_CUR); > > > > > > > > Which gives a `pos` value of 2. > > > Without unget: > > fgetc: Position in stream before fflush: 1424 > > fgetc: Position in stream after fflush: 2 > > That's the only value 2 in the test, and it's using fgetc, not fgets? > Correct, the difference with my original test resides in the content size of the file, here I tried with the source code, which has a size of 1424 bytes, while in my original test, the text file had a size of 3 bytes. My original assumption was that there was a difference by 1, but actually with fgets, the position seems to be set to file (buffer?) content size - 1. > > Thanks, > Florian > >
* Frederic Berat: >> That's the only value 2 in the test, and it's using fgetc, not fgets? > > Correct, the difference with my original test resides in the content > size of the file, here I tried with the source code, which has a size > of 1424 bytes, while in my original test, the text file had a size of > 3 bytes. My original assumption was that there was a difference by 1, > but actually with fgets, the position seems to be set to file > (buffer?) content size - 1. After the fflush or before the fflush? Again, without the actual test you are looking at, it's difficult to help you. Thanks, Florian
On Thu, Nov 7, 2024 at 11:40 AM Florian Weimer <fweimer@redhat.com> wrote: > * Frederic Berat: > > >> That's the only value 2 in the test, and it's using fgetc, not fgets? > > > > Correct, the difference with my original test resides in the content > > size of the file, here I tried with the source code, which has a size > > of 1424 bytes, while in my original test, the text file had a size of > > 3 bytes. My original assumption was that there was a difference by 1, > > but actually with fgets, the position seems to be set to file > > (buffer?) content size - 1. > > After the fflush or before the fflush? Again, without the actual test > you are looking at, it's difficult to help you. > > $> echo -n "0:1" > test.txt $> ./fflush_fgets test.txt Without unget: fgets: Position in stream before fflush: 3 fgets: Position in stream after fflush: 1 With unget: fgets: Position in stream before fflush: 3 fgets: Position in stream after fflush: 2 Without unget: fgetc: Position in stream before fflush: 3 fgetc: Position in stream after fflush: 2 With unget: fgetc: Position in stream before fflush: 3 fgetc: Position in stream after fflush: 1 > Thanks, > Florian > >
* Frederic Berat: > On Thu, Nov 7, 2024 at 11:40 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Frederic Berat: > > >> That's the only value 2 in the test, and it's using fgetc, not fgets? > > > > Correct, the difference with my original test resides in the content > > size of the file, here I tried with the source code, which has a size > > of 1424 bytes, while in my original test, the text file had a size of > > 3 bytes. My original assumption was that there was a difference by 1, > > but actually with fgets, the position seems to be set to file > > (buffer?) content size - 1. > > After the fflush or before the fflush? Again, without the actual test > you are looking at, it's difficult to help you. > > $> echo -n "0:1" > test.txt > $> ./fflush_fgets test.txt > Without unget: > fgets: Position in stream before fflush: 3 > fgets: Position in stream after fflush: 1 > With unget: > fgets: Position in stream before fflush: 3 > fgets: Position in stream after fflush: 2 > Without unget: > fgetc: Position in stream before fflush: 3 > fgetc: Position in stream after fflush: 2 > With unget: > fgetc: Position in stream before fflush: 3 > fgetc: Position in stream after fflush: 1 I made this change: void fgets_case (const char *argv[], int unget) { FILE *f = fopen(argv[1], "r+"); if (!f) { printf("Unable to open %s.\n", argv[1]); return; } char s[3] = ""; if (!fgets(s, 2, f)) { printf("Unable to retrieve string from f.\n"); fclose(f); return; } int off = ftello(f); if (unget) { printf("With unget:\n"); ungetc(s[1], f); } else { printf("Without unget:\n"); } printf ("\toffset after fgets: %d\n", off); printf(" fgets: Position in stream before fflush: %ld\n", lseek(fileno(f), 0, SEEK_CUR)); fflush(f); printf(" fgets: Position in stream after fflush: %ld\n", lseek(fileno(f), 0, SEEK_CUR)); printf ("\toffset after fflush: %d\n", (int) ftello(f)); fclose(f); } And I get: Without unget: offset after fgets: 1 fgets: Position in stream before fflush: 3 fgets: Position in stream after fflush: 1 offset after fflush: 1 With unget: offset after fgets: 1 fgets: Position in stream before fflush: 3 fgets: Position in stream after fflush: 2 offset after fflush: 0 As you are reading just one byte, file offset 1 before ungetc is correct. After ungetc, it should be 0. File descriptor and stream should have the same offset. So this looks like a genuine bug to me. Thanks, Florian
* Frédéric Bérat: > diff --git a/libio/Makefile b/libio/Makefile > index 4370152964..27534b9675 100644 > --- a/libio/Makefile > +++ b/libio/Makefile > @@ -100,6 +100,8 @@ tests = \ > tst-fclose-unopened \ > tst-fclose-unopened2 \ > tst-fdopen-seek-failure \ > + tst-fflush \ > + tst-fflush-NULL \ > tst-fgetc-after-eof \ > tst-fgetwc \ > tst-fgetws \ > @@ -146,6 +148,9 @@ tests = \ > tst_wscanf \ > # tests > > +# tst-fflush-NULL as XFAIL until read stream bug is fixed > +test-xfail-tst-fflush-NULL = yes Please file a new bug in Bugzilla and reference its number in the comment. > diff --git a/libio/tst-fflush-skeleton.c b/libio/tst-fflush-skeleton.c > new file mode 100644 > index 0000000000..5db9fc96da > --- /dev/null > +++ b/libio/tst-fflush-skeleton.c > @@ -0,0 +1,196 @@ > +#ifndef FILE_FLUSH_TYPE > +# define FILE_FLUSH_TYPE FILE_FLUSH > +# define S_FLUSH_TYPE "FILE" > +#endif I suggest to put this in to the libio/tst-fflush.c wrapper. > +struct > +{ > + FILE *file; > + char *name; > + int fd; > + char *mfile; > +} files[TEST_FILE_COUNT]; > + > +static void > +base_init (int file) > +{ > + files[file].file = NULL; > + files[file].fd = -1; > + files[file].name = NULL; > + files[file].mfile = NULL; > +} > + > +static void > +file_init (int file) > +{ > + int fd = -1; > + > + base_init (file); > + > + if (file >= TEST_FILE_COUNT) > + return; > + > + xclose (create_temp_file ("tst-fflush", &files[file].name)); > + > + fd = xopen (files[file].name, O_RDONLY, 0); > + files[file].mfile = (char *) xmmap (NULL, CONTENT_SZ_MAX, PROT_READ, > + MAP_SHARED, fd); > + xclose (fd); > +} > + > +static void > +file_cleanup (int file) > +{ > + free (files[file].name); > + xmunmap (files[file].mfile, CONTENT_SZ_MAX); > + > + base_init (file); > +} > + > +static int > +file_changed (int to_check, int global_flush, const char *mode) > +{ This could use FILE_FLUSH_TYPE directly. > + struct stat stats = { }; > + bool content_matches = 0; > + char expected[CONTENT_SZ_MAX] = { }; > + > + verbose_printf ("Check that %s (%d) exactly contains the data we put in\n", > + files[to_check].name, to_check); > + > + /* File should contain "N:M" where both N and M are one digit exactly. */ > + snprintf (expected, sizeof (expected), "%d:%d", global_flush, to_check); > + content_matches > + = (strncmp (files[to_check].mfile, expected, sizeof (expected)) == 0); I think this should use memcmp, not strncmp? So perhaps TEST_COMPARE_BLOB (files[to_check].mfile, sizeof (expected), expected, sizeof (expected)); > + TEST_VERIFY_EXIT (content_matches != 0); Maybe: if (support_record_failure_is_failed) FAIL_EXIT1 ("exiting due to previous failure"); We should probably something like support_record_failure_barrier (); for that. > + TEST_VERIFY_EXIT (fstat (files[to_check].fd, &stats) >= 0); > + TEST_VERIFY_EXIT (stats.st_size == 3); > + if (strncmp(mode, "r", 1) == 0) Missing space before “(mode”. Alternatively, just use: mode[0] == 'r' > + TEST_VERIFY_EXIT (lseek (files[to_check].fd, 0, SEEK_CUR) == 1); > + else > + TEST_VERIFY_EXIT (lseek (files[to_check].fd, 0, SEEK_CUR) == 3); I think the exit isn't required, so maybe (with a comment)? TEST_COMPARE (lseek (files[to_check].fd, 0, SEEK_CUR), mode[0] == 'r' ? 1 : 3); > + /* Not reached if the data doesn't match. */ > + return FILE_CHANGED; > +} > + > +static void > +file_flush (int global_flush, const char *mode) > +{ > + for (int i = 0; i < TEST_FILE_COUNT; i++) { “{” should be on its own line. > + files[i].file = xfopen (files[i].name, mode); > + TEST_VERIFY_EXIT (files[i].file != NULL); > + files[i].fd = fileno (files[i].file); > + } > + > + /* Print a unique identifier in each file, that is not too long nor contain > + new line to not trigger _IO_OVERFLOW/_IO_SYNC. */ > + for (int i = 0; i < TEST_FILE_COUNT; i++) { Likewise. > + if (strncmp(mode, "r", 1) == 0) Missing space, or even better: mode[0] == 'r' > + { > + char c = (char) fgetc (files[i].file); > + c = (char) fgetc (files[i].file); > + ungetc (c, files[i].file); Please make the ungetc conditional and run this subtest twice. > + } > + else > + { > + fprintf (files[i].file, "%d:%d", global_flush, i); > + } > + } > + > + if (global_flush) Could use FILE_FLUSH_TYPE directly. > + { > + fflush (NULL); > + } > + else > + { > + for (int i = 0; i < TEST_FILE_COUNT; i++) > + fflush (files[i].file); > + } Missing checks for fflush return values. Overall structure of the test looks reasonable to me, thanks. Florian
diff --git a/libio/Makefile b/libio/Makefile index 4370152964..27534b9675 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -100,6 +100,8 @@ tests = \ tst-fclose-unopened \ tst-fclose-unopened2 \ tst-fdopen-seek-failure \ + tst-fflush \ + tst-fflush-NULL \ tst-fgetc-after-eof \ tst-fgetwc \ tst-fgetws \ @@ -146,6 +148,9 @@ tests = \ tst_wscanf \ # tests +# tst-fflush-NULL as XFAIL until read stream bug is fixed +test-xfail-tst-fflush-NULL = yes + $(objpfx)tst-popen-fork: $(shared-thread-library) tests-internal = tst-vtables tst-vtables-interposed diff --git a/libio/tst-fflush-NULL.c b/libio/tst-fflush-NULL.c new file mode 100644 index 0000000000..23c741e0dc --- /dev/null +++ b/libio/tst-fflush-NULL.c @@ -0,0 +1,23 @@ +/* Test that fflush (FILE) and fflush (NULL) are semantically equivalent. + + Copyright (C) 2024 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 + <https://www.gnu.org/licenses/>. */ + +#define FILE_FLUSH_TYPE 1 +#define S_FLUSH_TYPE "NULL" + +#include "tst-fflush-skeleton.c" diff --git a/libio/tst-fflush-skeleton.c b/libio/tst-fflush-skeleton.c new file mode 100644 index 0000000000..5db9fc96da --- /dev/null +++ b/libio/tst-fflush-skeleton.c @@ -0,0 +1,196 @@ +/* Test that fflush (FILE) and fflush (NULL) are semantically equivalent. + + Copyright (C) 2024 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 + <https://www.gnu.org/licenses/>. */ + +/* A success on this test doesn't imply the effectiveness of fflush as + we can't ensure that the file wasn't already in the expected state + before the call of the function. It only ensures that, if the test + fails, fflush is broken. */ + +#include <fcntl.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#include <sys/stat.h> +#include <sys/mman.h> + +#include <support/check.h> +#include <support/support.h> +#include <support/temp_file.h> +#include <support/test-driver.h> +#include <support/xstdio.h> +#include <support/xunistd.h> + +#define CONTENT_SZ_MAX 32 +#define TEST_FILE_COUNT 10 + +#define FILE_FLUSH 0 +#define GLOBAL_FLUSH 1 + +#ifndef FILE_FLUSH_TYPE +# define FILE_FLUSH_TYPE FILE_FLUSH +# define S_FLUSH_TYPE "FILE" +#endif + +#define FILE_UNCHANGED 0 +#define FILE_CHANGED 1 + +struct +{ + FILE *file; + char *name; + int fd; + char *mfile; +} files[TEST_FILE_COUNT]; + +static void +base_init (int file) +{ + files[file].file = NULL; + files[file].fd = -1; + files[file].name = NULL; + files[file].mfile = NULL; +} + +static void +file_init (int file) +{ + int fd = -1; + + base_init (file); + + if (file >= TEST_FILE_COUNT) + return; + + xclose (create_temp_file ("tst-fflush", &files[file].name)); + + fd = xopen (files[file].name, O_RDONLY, 0); + files[file].mfile = (char *) xmmap (NULL, CONTENT_SZ_MAX, PROT_READ, + MAP_SHARED, fd); + xclose (fd); +} + +static void +file_cleanup (int file) +{ + free (files[file].name); + xmunmap (files[file].mfile, CONTENT_SZ_MAX); + + base_init (file); +} + +static int +file_changed (int to_check, int global_flush, const char *mode) +{ + struct stat stats = { }; + bool content_matches = 0; + char expected[CONTENT_SZ_MAX] = { }; + + verbose_printf ("Check that %s (%d) exactly contains the data we put in\n", + files[to_check].name, to_check); + + /* File should contain "N:M" where both N and M are one digit exactly. */ + snprintf (expected, sizeof (expected), "%d:%d", global_flush, to_check); + content_matches + = (strncmp (files[to_check].mfile, expected, sizeof (expected)) == 0); + + TEST_VERIFY_EXIT (content_matches != 0); + + TEST_VERIFY_EXIT (fstat (files[to_check].fd, &stats) >= 0); + TEST_VERIFY_EXIT (stats.st_size == 3); + if (strncmp(mode, "r", 1) == 0) + TEST_VERIFY_EXIT (lseek (files[to_check].fd, 0, SEEK_CUR) == 1); + else + TEST_VERIFY_EXIT (lseek (files[to_check].fd, 0, SEEK_CUR) == 3); + + + /* Not reached if the data doesn't match. */ + return FILE_CHANGED; +} + +static void +file_flush (int global_flush, const char *mode) +{ + for (int i = 0; i < TEST_FILE_COUNT; i++) { + files[i].file = xfopen (files[i].name, mode); + TEST_VERIFY_EXIT (files[i].file != NULL); + files[i].fd = fileno (files[i].file); + } + + /* Print a unique identifier in each file, that is not too long nor contain + new line to not trigger _IO_OVERFLOW/_IO_SYNC. */ + for (int i = 0; i < TEST_FILE_COUNT; i++) { + if (strncmp(mode, "r", 1) == 0) + { + char c = (char) fgetc (files[i].file); + c = (char) fgetc (files[i].file); + ungetc (c, files[i].file); + } + else + { + fprintf (files[i].file, "%d:%d", global_flush, i); + } + } + + if (global_flush) + { + fflush (NULL); + } + else + { + for (int i = 0; i < TEST_FILE_COUNT; i++) + fflush (files[i].file); + } + + for (int i = 0; i < TEST_FILE_COUNT; i++) + { + int changed = file_changed (i, global_flush, mode); + + verbose_printf ("Check that file %s has been modified after fflush\n", + files[i].name); + verbose_printf ("File %s; changed %d; global fflush %d\n", + files[i].name, changed, !!global_flush); + TEST_VERIFY_EXIT (changed != FILE_UNCHANGED); + } + + for (int i = 0; i < TEST_FILE_COUNT; i++) + xfclose (files[i].file); +} + +static int +do_test (void) +{ + for (int i = 0; i < TEST_FILE_COUNT; i++) + file_init (i); + + verbose_printf ("Checking fflush(" S_FLUSH_TYPE "), WRITE mode\n"); + file_flush (FILE_FLUSH_TYPE, "w"); + + verbose_printf ("Checking fflush(" S_FLUSH_TYPE "), READWRITE mode\n"); + file_flush (FILE_FLUSH_TYPE, "r+"); + + for (int i = 0; i < TEST_FILE_COUNT; i++) + file_cleanup (i); + + return 0; +} + +#include <support/test-driver.c> diff --git a/libio/tst-fflush.c b/libio/tst-fflush.c new file mode 100644 index 0000000000..525ed0f1f2 --- /dev/null +++ b/libio/tst-fflush.c @@ -0,0 +1,20 @@ +/* Test that fflush (FILE) and fflush (NULL) are semantically equivalent. + + Copyright (C) 2024 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 + <https://www.gnu.org/licenses/>. */ + +#include "tst-fflush-skeleton.c"