This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v5][BZ #12685] Allow arbitrary number of modes in fopen.
- From: Roland McGrath <roland at hack dot frob dot com>
- To: OndÅej BÃlka <neleai at seznam dot cz>
- Cc: Andreas Schwab <schwab at suse dot de>, Carlos O'Donell <carlos at redhat dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>, libc-alpha at sourceware dot org
- Date: Wed, 29 May 2013 15:19:58 -0700 (PDT)
- Subject: Re: [PATCH v5][BZ #12685] Allow arbitrary number of modes in fopen.
- References: <20130510103330 dot GA12338 at domone dot kolej dot mff dot cuni dot cz> <Pine dot LNX dot 4 dot 64 dot 1305101139470 dot 1189 at digraph dot polyomino dot org dot uk> <20130514121540 dot GA28892 at domone dot kolej dot mff dot cuni dot cz> <mvm38tpzqst dot fsf at hawking dot suse dot de> <20130514124257 dot GB18821 at domone dot kolej dot mff dot cuni dot cz> <mvmy5bhyb4h dot fsf at hawking dot suse dot de> <20130515091735 dot GA22641 at domone dot kolej dot mff dot cuni dot cz> <5193F463 dot 3040501 at redhat dot com> <20130520081258 dot GA32195 at domone dot kolej dot mff dot cuni dot cz> <mvmwqqspz50 dot fsf at hawking dot suse dot de> <20130524131333 dot GB10400 at domone dot kolej dot mff dot cuni dot cz>
> Thanks, here is v5.
This posting does not include ChangeLog text.
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 61b61b3..78a269a 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -286,12 +286,10 @@ _IO_new_file_fopen (fp, filename, mode, is32not64)
> #ifdef _LIBC
> last_recognized = mode;
> #endif
> - for (i = 1; i < 7; ++i)
> + while (*++mode != '\0' && *mode != ',')
> {
> - switch (*++mode)
> + switch (*mode)
> {
> - case '\0':
> - break;
> case '+':
> omode = O_RDWR;
> read_write &= _IO_IS_APPENDING;
> diff --git a/libio/iofdopen.c b/libio/iofdopen.c
> index a65a5a6..db646c0 100644
> --- a/libio/iofdopen.c
> +++ b/libio/iofdopen.c
> @@ -75,12 +75,10 @@ _IO_new_fdopen (fd, mode)
> MAYBE_SET_EINVAL;
> return NULL;
> }
> - for (i = 1; i < 5; ++i)
> + while (*++mode != '\0' && *mode != ',')
> {
> - switch (*++mode)
> + switch (*mode)
> {
> - case '\0':
> - break;
> case '+':
> read_write &= _IO_IS_APPENDING;
> break;
The actual change here looks fine. Perhaps you can file a bug or add an
item on a wiki list to refactor these into shared code for parsing the mode
string.
> +++ b/libio/test-fopen-bz12685.c
We haven't been very consistent, but we sort of have a convention of using
"bug-" rather than "test-" or "tst-" for tests that are individual
regression tests. I think I'd also rather see a name that indicates
directly what it's testing, instead of having BZ# in the file name.
e.g. "bug-openmodes-parsing.c".
> + int i;
Unused variable. You should have gotten a warning about that, and you
should always be looking to make sure you don't introduce new warnings.
> + FILE *stream, *stream2;
stream2 is unused. For the variable that needs to exist, don't declare it
ahead of time, just use a C99-style initializing definition at the place in
the code where you first set it.
> + /* Test if fopen mode e that sets FD_CLOEXEC is recognized. */
> + stream = fopen ("/dev/null", "rccccccccccccccccccccce");
> +
> + if ((fcntl (fileno (stream), F_GETFD) & FD_CLOEXEC) == 0)
This relies on lower-level stuff that really isn't necessary to test this.
Instead, use a mode that ends with "+" and then attempt a call like putc.
> + printf("no FD_CLOEXEC on fopen\n");
Space before paren.
> + return 1;
> + }
> + /* In fdopen e flag is ignored. */
The "+" method works for fdopen too, so you can test both changes.
Thanks,
Roland