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 <[email protected]>
> +
> +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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "[email protected]"

Reply via email to