Eric Blake wrote:
> I see a few changes to squash in:
I see some more changes to squash in:
> +#include <sys/stat.h>
This requires the use of module 'sys_stat'. (See in
doc/posix-headers/sys_stat.texi; we use S_REG.)
> +/* Get SIZE_MAX. */
> +#include <stdint.h>
This requires the use of module 'stdint'.
> /* Get realloc, free. */
> #include <stdlib.h>
This is now also used to get 'malloc'.
> @@ -38,6 +49,33 @@ fread_file (FILE * stream, size_t * length)
> size_t alloc = 0;
> size_t size = 0;
> int save_errno;
> + struct stat st;
The number of variables declared at the beginning of this function.
As a general rule, try to minimize the scope of local variables.
> + do
> + {
> + off_t alloc_off, pos;
> +
> + if (fstat (fileno (stream), &st) < 0 || !S_ISREG (st.st_mode))
> + break;
> +
> + pos = ftello (stream);
> + if (pos < 0 || pos > st.st_size)
> + break;
> +
> + alloc_off = st.st_size - pos;
> + if (SIZE_MAX <= alloc_off)
> + {
> + errno = ENOMEM;
> + return NULL;
> + }
> +
> + alloc = alloc_off + 1;
> +
> + buf = malloc (alloc);
> + if (!buf)
> + return NULL;
> + }
> + while (0);
The control flow is obfuscated by using a do { } while (0) and several 'break'
statements that act as 'goto' statements. It would be much clearer with 'if'
statements.
> if (size + BUFSIZ + 1 > alloc)
> {
> char *new_buf;
> + size_t new_alloc = alloc + alloc / 2;
> +
> + if (new_alloc < alloc)
> + {
> + save_errno = ENOMEM;
> + break;
> + }
I see not a single comment here. Please put a comment every 10 lines on
average.
I'm committing this combined patch.
2010-08-28 Giuseppe Scrivano <gscriv...@gnu.org>
Eric Blake <ebl...@redhat.com>
Bruno Haible <br...@clisp.org>
read-file: Avoid memory reallocations with regular files.
* lib/read-file.c: Include <sys/stat.h>, <stdio.h>, <stdint.h>.
(fread_file): With regular files, use the remaining length as the
initial buffer size. Check against overflow.
* modules/read-file (Depends-on): Add ftello, malloc-posix, stdint,
sys_stat.
*** lib/read-file.c.orig Sat Aug 28 16:11:04 2010
--- lib/read-file.c Sat Aug 28 16:10:39 2010
***************
*** 20,26 ****
#include "read-file.h"
! /* Get realloc, free. */
#include <stdlib.h>
/* Get errno. */
--- 20,35 ----
#include "read-file.h"
! /* Get fstat. */
! #include <sys/stat.h>
!
! /* Get ftello. */
! #include <stdio.h>
!
! /* Get SIZE_MAX. */
! #include <stdint.h>
!
! /* Get malloc, realloc, free. */
#include <stdlib.h>
/* Get errno. */
***************
*** 36,85 ****
{
char *buf = NULL;
size_t alloc = 0;
- size_t size = 0;
- int save_errno;
! for (;;)
! {
! size_t count;
! size_t requested;
!
! if (size + BUFSIZ + 1 > alloc)
! {
! char *new_buf;
!
! alloc += alloc / 2;
! if (alloc < size + BUFSIZ + 1)
! alloc = size + BUFSIZ + 1;
!
! new_buf = realloc (buf, alloc);
! if (!new_buf)
! {
! save_errno = errno;
break;
! }
!
! buf = new_buf;
! }
!
! requested = alloc - size - 1;
! count = fread (buf + size, 1, requested, stream);
! size += count;
!
! if (count != requested)
! {
! save_errno = errno;
! if (ferror (stream))
! break;
! buf[size] = '\0';
! *length = size;
! return buf;
! }
! }
!
! free (buf);
! errno = save_errno;
! return NULL;
}
static char *
--- 45,134 ----
{
char *buf = NULL;
size_t alloc = 0;
! /* For a regular file, allocate a buffer that has exactly the right
! size. This avoids the need to do dynamic reallocations later. */
! {
! struct stat st;
!
! if (fstat (fileno (stream), &st) >= 0 && S_ISREG (st.st_mode))
! {
! off_t pos = ftello (stream);
!
! if (pos >= 0 && pos < st.st_size)
! {
! off_t alloc_off = st.st_size - pos;
!
! if (SIZE_MAX <= alloc_off)
! {
! errno = ENOMEM;
! return NULL;
! }
!
! alloc = alloc_off + 1;
!
! buf = malloc (alloc);
! if (!buf)
! /* errno is ENOMEM. */
! return NULL;
! }
! }
! }
!
! {
! size_t size = 0; /* number of bytes read so far */
! int save_errno;
!
! for (;;)
! {
! size_t count;
! size_t requested;
!
! if (size + BUFSIZ + 1 > alloc)
! {
! char *new_buf;
! size_t new_alloc = alloc + alloc / 2;
!
! /* Check against overflow. */
! if (new_alloc < alloc)
! {
! save_errno = ENOMEM;
! break;
! }
!
! alloc = new_alloc;
! if (alloc < size + BUFSIZ + 1)
! alloc = size + BUFSIZ + 1;
!
! new_buf = realloc (buf, alloc);
! if (!new_buf)
! {
! save_errno = errno;
! break;
! }
!
! buf = new_buf;
! }
!
! requested = alloc - size - 1;
! count = fread (buf + size, 1, requested, stream);
! size += count;
!
! if (count != requested)
! {
! save_errno = errno;
! if (ferror (stream))
break;
! buf[size] = '\0';
! *length = size;
! return buf;
! }
! }
!
! free (buf);
! errno = save_errno;
! return NULL;
! }
}
static char *
*** modules/read-file.orig Sat Aug 28 16:11:04 2010
--- modules/read-file Sat Aug 28 15:47:24 2010
***************
*** 7,13 ****
--- 7,17 ----
m4/read-file.m4
Depends-on:
+ ftello
+ malloc-posix
realloc-posix
+ stdint
+ sys_stat
configure.ac:
gl_FUNC_READ_FILE
