svn commit: r246120 - in head: include lib/libc/stdio tools/regression/lib/libc/stdio

[John Baldwin]

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 at 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