From owner-svn-src-all@FreeBSD.ORG Thu Jan 31 17:04:19 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 9B6C1FD3; Thu, 31 Jan 2013 17:04:19 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 5D16A387; Thu, 31 Jan 2013 17:04:19 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 44DF7B922; Thu, 31 Jan 2013 12:04:18 -0500 (EST) From: John Baldwin To: Pietro Cerutti Subject: Re: svn commit: r246120 - in head: include lib/libc/stdio tools/regression/lib/libc/stdio Date: Thu, 31 Jan 2013 11:23:08 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64; ; ) References: <201301301459.r0UExQKw074249@svn.freebsd.org> In-Reply-To: <201301301459.r0UExQKw074249@svn.freebsd.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201301311123.09015.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 31 Jan 2013 12:04:18 -0500 (EST) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Jan 2013 17:04:19 -0000 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 > + > +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