Date: Thu, 31 Jan 2013 00:54:32 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Pietro Cerutti <gahr@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r246120 - in head: include lib/libc/stdio tools/regression/lib/libc/stdio Message-ID: <20130130235432.GA24750@stack.nl> In-Reply-To: <201301301459.r0UExQKw074249@svn.freebsd.org> References: <201301301459.r0UExQKw074249@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jan 30, 2013 at 02:59:26PM +0000, Pietro Cerutti wrote: > Author: gahr (ports committer) > Date: Wed Jan 30 14:59:26 2013 > New Revision: 246120 > URL: http://svnweb.freebsd.org/changeset/base/246120 > Log: > Add fmemopen(3), an interface to get a FILE * from a buffer in memory, along > with the respective regression test. > See http://pubs.opengroup.org/onlinepubs/9699919799/functions/fmemopen.html Various comments are inline. > Added: > head/lib/libc/stdio/fmemopen.c (contents, props changed) > head/tools/regression/lib/libc/stdio/test-fmemopen.c (contents, props changed) > head/tools/regression/lib/libc/stdio/test-fmemopen.t (contents, props changed) > Modified: > head/include/stdio.h (contents, props changed) > head/lib/libc/stdio/Makefile.inc (contents, props changed) > head/lib/libc/stdio/Symbol.map (contents, props changed) > head/lib/libc/stdio/fopen.3 (contents, props changed) > Modified: head/lib/libc/stdio/Makefile.inc > ============================================================================== > --- head/lib/libc/stdio/Makefile.inc Wed Jan 30 13:14:34 2013 (r246119) > +++ head/lib/libc/stdio/Makefile.inc Wed Jan 30 14:59:26 2013 (r246120) > @@ -8,7 +8,8 @@ SRCS+= _flock_stub.c asprintf.c clrerr.c > fclose.c fcloseall.c fdopen.c \ > feof.c ferror.c fflush.c fgetc.c fgetln.c fgetpos.c fgets.c fgetwc.c \ > fgetwln.c fgetws.c \ > - fileno.c findfp.c flags.c fopen.c fprintf.c fpurge.c fputc.c fputs.c \ > + fileno.c findfp.c flags.c fmemopen.c fopen.c fprintf.c fpurge.c \ > + fputc.c fputs.c \ > fputwc.c fputws.c fread.c freopen.c fscanf.c fseek.c fsetpos.c \ > ftell.c funopen.c fvwrite.c fwalk.c fwide.c fwprintf.c fwscanf.c \ > fwrite.c getc.c getchar.c getdelim.c getline.c \ > @@ -48,7 +49,7 @@ MLINKS+=ferror.3 ferror_unlocked.3 \ > MLINKS+=fflush.3 fpurge.3 > MLINKS+=fgets.3 gets.3 > MLINKS+=flockfile.3 ftrylockfile.3 flockfile.3 funlockfile.3 > -MLINKS+=fopen.3 fdopen.3 fopen.3 freopen.3 > +MLINKS+=fopen.3 fdopen.3 fopen.3 freopen.3 fopen.3 fmemopen.3 > MLINKS+=fputs.3 puts.3 > MLINKS+=fread.3 fwrite.3 > MLINKS+=fseek.3 fgetpos.3 fseek.3 fseeko.3 fseek.3 fsetpos.3 fseek.3 ftell.3 \ I think the man page could be a separate page without problem. There is a fair amount of behaviour unique to fmemopen(). > Added: head/lib/libc/stdio/fmemopen.c > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/lib/libc/stdio/fmemopen.c Wed Jan 30 14:59:26 2013 (r246120) > @@ -0,0 +1,182 @@ > +/*- > +Copyright (C) 2013 Pietro Cerutti <gahr@FreeBSD.org> > + > +Redistribution and use in source and binary forms, with or without > +modification, are permitted provided that the following conditions > +are met: > +1. Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > +2. Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + > +THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND > +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > +ARE DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE > +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > +OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > +HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > +LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > +OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > +SUCH DAMAGE. > +*/ > + > +#include <sys/cdefs.h> > +__FBSDID("$FreeBSD$"); > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <errno.h> > + > +struct __fmemopen_cookie I think the underscores are not necessary if the name cannot collide with a user's name. > +{ > + char *buf; /* pointer to the memory region */ > + char own; /* did we allocate the buffer ourselves? */ > + long len; /* buffer length in bytes */ This is in fact a size_t, as passed to fmemopen(). > + long off; /* current offset into the buffer */ This should probably be a size_t as well. > +}; > + > +static int fmemopen_read (void *cookie, char *buf, int nbytes); > +static int fmemopen_write (void *cookie, const char *buf, int nbytes); > +static fpos_t fmemopen_seek (void *cookie, fpos_t offset, int whence); > +static int fmemopen_close (void *cookie); > + > +FILE * > +fmemopen (void * __restrict buf, size_t size, const char * __restrict mode) > +{ > + /* allocate cookie */ > + struct __fmemopen_cookie *ck = malloc (sizeof (struct __fmemopen_cookie)); > + if (ck == NULL) { > + errno = ENOMEM; malloc() already sets errno when it fails; you need not do it yourself. > + return (NULL); > + } > + > + ck->off = 0; > + ck->len = size; > + > + /* do we have to allocate the buffer ourselves? */ > + ck->own = ((ck->buf = buf) == NULL); > + if (ck->own) { > + ck->buf = malloc (size); > + if (ck->buf == NULL) { > + free (ck); > + errno = ENOMEM; > + return (NULL); > + } > + ck->buf[0] = '\0'; > + } > + > + if (mode[0] == 'a') > + ck->off = strnlen(ck->buf, ck->len); This is the only use of 'mode'. POSIX mentions a difference between 'r' and 'w' with regard to another piece of internal state: the notion of the size of the current buffer contents, which seems completely missing here. There was an interpretation issued about that as well, http://austingroupbugs.net/view.php?id=587 . glibc does something different when the mode specifies binary ('b') and POSIX will likely require this in future versions, see http://austingroupbugs.net/view.php?id=456 . It is probably best to fail with [EINVAL] if this special behaviour is not implemented so that people are not surprised. > + > + /* actuall wrapper */ > + FILE *f = funopen ((void *)ck, fmemopen_read, fmemopen_write, > + fmemopen_seek, fmemopen_close); The read and write functions should be passed as NULL instead when the mode does not permit reading or writing respectively. > + > + if (f == NULL) { > + if (ck->own) > + free (ck->buf); > + free (ck); > + return (NULL); > + } > + > + /* turn off buffering, so a write past the end of the buffer > + * correctly returns a short object count */ > + setvbuf (f, (char *) NULL, _IONBF, 0); This is indeed required but I think it makes the stream rather slow (because it will call fmemopen_read/fmemopen_write for single bytes). If this is a problem, I think it should be fixed by making unbuffered mode smarter in the rest of stdio and not here. > + > + return (f); > +} > + > +static int > +fmemopen_read (void *cookie, char *buf, int nbytes) stdio is strange, requiring 'int' here :( > +{ > + struct __fmemopen_cookie *ck = cookie; > + > + if (nbytes > ck->len - ck->off) > + nbytes = ck->len - ck->off; > + > + if (nbytes == 0) > + return (0); > + > + memcpy (buf, ck->buf + ck->off, nbytes); > + > + ck->off += nbytes; > + > + return (nbytes); > +} > [snip] > Modified: head/lib/libc/stdio/fopen.3 > ============================================================================== > --- head/lib/libc/stdio/fopen.3 Wed Jan 30 13:14:34 2013 (r246119) > +++ head/lib/libc/stdio/fopen.3 Wed Jan 30 14:59:26 2013 (r246120) > @@ -32,13 +32,14 @@ > [snip] > @@ -202,6 +205,29 @@ standard text stream > .Dv ( stderr , stdin , > or > .Dv stdout ) . > +.Pp > +The > +.Fn fmemopen > +function > +associates the buffer given by the > +.Fa buf > +and > +.Fa size > +arguments with a stream. > +The > +.Fa buf > +argument shall be either a null pointer or point to a buffer that > +is at least > +.Fa size > +bytes long. > +If a null pointer is specified as the > +.Fa buf > +argument, > +.Fn fmemopen > +shall allocate We don't usually use 'shall' for requirements on the implementation in man pages. We made it such that it is compliant; thus, it is, not shall be. > [snip] > @@ -294,3 +322,8 @@ The > .Dq Li e > mode option does not conform to any standard > but is also supported by glibc. > +The > +.Fn fmemopen > +function > +conforms to > +.St -p1003.1-2008 . Quite a bit of functionality is missing. > Added: head/tools/regression/lib/libc/stdio/test-fmemopen.c > ============================================================================== > --- /dev/null 00:00:00 1970 (empty, because file is newly added) > +++ head/tools/regression/lib/libc/stdio/test-fmemopen.c Wed Jan 30 14:59:26 2013 (r246120) > @@ -0,0 +1,143 @@ > +/*- > +Copyright (C) 2013 Pietro Cerutti <gahr@FreeBSD.org> > + > +Redistribution and use in source and binary forms, with or without > +modification, are permitted provided that the following conditions > +are met: > +1. Redistributions of source code must retain the above copyright > + notice, this list of conditions and the following disclaimer. > +2. Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + > +THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND > +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > +ARE DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE > +FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > +DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > +OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > +HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > +LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > +OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > +SUCH DAMAGE. > +*/ > + > +/* > + * Test basic FILE * functions (fread, fwrite, fseek, fclose) against > + * a FILE * retrieved using fmemopen() > + */ > + > +#include <sys/cdefs.h> > +__FBSDID("$FreeBSD$"); > + > +#include <assert.h> > +#include <errno.h> > +#include <stdio.h> > +#include <string.h> > +#include <strings.h> > + > [snip] > +void > +test_autoalloc () > +{ > + /* > + * let fmemopen allocate the buffer > + */ > + > + char str[] = "A quick test"; > + FILE *fp; > + long pos; > + size_t nofw, nofr, i; > + int rc; > + > + /* open a FILE * using fmemopen */ > + fp = fmemopen (NULL, 512, "w"); This does not make sense in a real program: there is no way to read the data back. POSIX permits fmemopen(NULL, ...) to fail when the mode is read-only or write-only. > + assert (fp != NULL); > + > + /* fill the buffer */ > + for (i = 0; i < 512; i++) { > + nofw = fwrite ("a", 1, 1, fp); > + assert (nofw == 1); > + } > + > + /* get the current position into the stream */ > + pos = ftell (fp); > + assert (pos == 512); > + > + /* try to write past the end, we should get a short object count (0) */ > + nofw = fwrite ("a", 1, 1, fp); > + assert (nofw == 0); > + > + /* close the FILE * */ > + rc = fclose (fp); > + assert (rc == 0); > +} > [snip] -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130130235432.GA24750>