Re: [PATCH] read-file: Avoid memory reallocations with seekable files.

[Eric Blake]

On 08/18/2010 05:43 AM, Giuseppe Scrivano wrote:
> PING^2
>
> are there more problems with this patch?
>
> > Subject: [PATCH] read-file: Avoid memory reallocations with regular files.
> >
> > * modules/read-file (Depends-on): Add ftello and malloc-posix.
> > * lib/read-file.c: Include<sys/types.h>,<sys/stat.h>,
> > <unistd.h>,<stdio.h>,<stdint.h>.
> > (fread_file): With regular files, use the remaining length as the
> > initial buffer size.  Check against overflow.

I see a few changes to squash in:

> > +/* Get fstat.  */
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>

POSIX (and therefore gnulib) guarantees that <sys/stat.h> is sufficient 
for fstat without explicitly listing <sys/types.h>.  And I didn't see 
any use of <unistd.h> in your additions.

> > +
> > +/* Get ftello.  */
> > +#include <stdio.h>
> > +
> > +/* Get SIZE_MAX.  */
> > +#include <stdint.h>
> > +
> >   /* Get realloc, free. */
> >   #include <stdlib.h>
> >
> > @@ -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;
> > +
> > +  do
> > +    {
> > +      off_t alloc_off, pos;

GNU style prefers listing each declaration on a separate line.

Also, tests/test-read-file should probably add a test for reading a 
FILE* that wraps a pipe in addition to its current tests of seekable 
files, to make sure we don't regress on non-seekable files (perhaps by 
adding test-read-file.sh, and making test-read-file.c consume stdin for 
several file types tied to stdin).

More problematic is this, which prevented me from pushing your patch:

> gnulib-tool: warning: module read-file depends on a module with an 
incompatible license: ftello

Before your change can be applied, we'd need consensus that ftello 
should be LGPLv2+ instead of its current LGPLv3+.  I think that boils 
down to just Bruno and myself, and I'm okay with the idea.



diff --git i/lib/read-file.c w/lib/read-file.c
index 628def1..34156bf 100644
--- i/lib/read-file.c
+++ w/lib/read-file.c
@@ -21,9 +21,7 @@
 #include "read-file.h"

 /* Get fstat.  */
-#include <sys/types.h>
 #include <sys/stat.h>
-#include <unistd.h>

 /* Get ftello.  */
 #include <stdio.h>
@@ -53,7 +51,8 @@ fread_file (FILE * stream, size_t * length)

   do
     {
-      off_t alloc_off, pos;
+      off_t alloc_off;
+      off_t pos;

       if (fstat (fileno (stream), &st) < 0 || !S_ISREG (st.st_mode))
         break;

--
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org