Date: Thu, 31 Jan 2013 11:23:08 -0500 From: John Baldwin <jhb@freebsd.org> 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: <201301311123.09015.jhb@freebsd.org> 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 Wednesday, January 30, 2013 9:59:26 am 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 > > Reviewed by: cognet > Approved by: cognet A few style suggestions: > 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: This is atypical style for license blocks. They always have a leading ' *' in other C files. In as much as possible this should match the template in /usr/share/examples/etc/bsd-style-copyright as much as possible. > +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); BSD style does not have spaces between function names and the list of arguments both in prototypes and calls. > + > +FILE * > +fmemopen (void * __restrict buf, size_t size, const char * __restrict mode) > +{ > + /* allocate cookie */ Banal comment (this is already obvious from the code). > + struct __fmemopen_cookie *ck = malloc (sizeof (struct __fmemopen_cookie)); Extra spaces after 'malloc' and 'sizeof'. Bruce also generally frowns upon assignments in declarations. > + if (ck == NULL) { > + errno = ENOMEM; > + return (NULL); > + } > + > + ck->off = 0; > + ck->len = size; > + > + /* do we have to allocate the buffer ourselves? */ Capitalize 'do' here as this is a sentence. (Comments should be full sentences when possible) > + ck->own = ((ck->buf = buf) == NULL); > + if (ck->own) { > + ck->buf = malloc (size); > + if (ck->buf == NULL) { > + free (ck); Not sure if you should save errno around free (once you let the malloc() error fall through per jilles@ notes). > + errno = ENOMEM; > + return (NULL); > + } > + ck->buf[0] = '\0'; > + } > + > + if (mode[0] == 'a') > + ck->off = strnlen(ck->buf, ck->len); > + > + /* actuall wrapper */ s/actuall/actual/, but I would just remove this comment instead. > + FILE *f = funopen ((void *)ck, fmemopen_read, fmemopen_write, > + fmemopen_seek, fmemopen_close); Cast to (void *) is not required in ANSI C and is just ugly. > + /* turn off buffering, so a write past the end of the buffer > + * correctly returns a short object count */ Please format this per style(9): /* * Turn off buffering so a write past the end of the buffer * correctl returns a short object count. */ > + setvbuf (f, (char *) NULL, _IONBF, 0); Note that a user can override this by calling setvbuf() on the returned FILE object. Not sure that is worth obsessing over. > +static int > +fmemopen_read (void *cookie, char *buf, int nbytes) > +{ > + 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); I would probably trim the blank lines here a bit. Certainly between the memcpy() and ck->off assignment, but I would probably trim this down to just one blank line between the variable declarations and the code body. Similar in other places. > +static fpos_t > +fmemopen_seek (void *cookie, fpos_t offset, int whence) > +{ > + struct __fmemopen_cookie *ck = cookie; > + > + > + switch (whence) { > + case SEEK_SET: > + if (offset > ck->len) { > + errno = EINVAL; > + return (-1); This should return POS_ERR on failure. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301311123.09015.jhb>