std.file.read implementation contest

[Steven Schveighoffer]

"Andrei Alexandrescu" wrote
> Someone mentioned an old bug in std.file.read here:
>
> http://www.reddit.com/r/programming/comments/7xnty/walter_bright_on_porting_d_to_the_mac/
>
> Two programmers sent in patches for the function. Which is to be committed 
> and why? (Linux versions shown. Apologies for noisy line breaks.)

Implementation 2, save 1 bug:

> Andrei
>
>
> // Implementation 1
...

>     else
>     {
> buf = GC.malloc(size, GC.BlkAttr.NO_SCAN)[0 .. size];
> enforce(buf, "Out of memory");
> scope(failure) delete buf;
>
> cenforce(std.c.linux.linux.read(fd, buf.ptr, size) == size, name);
>
> return buf[0 .. size];
>     }
> }

This doesn't work for /sys files which consistently say they are 4096 bytes 
large, even though they can be smaller or larger (encountered this with 
Tango also).

I think you have to kill the check that the size read actually equals the 
size in stat, AND you have to continue reading even after you reach that 
size.

>
> // Implementation 2
> void[] read(string name)
> {
>     immutable fd = std.c.linux.linux.open(toStringz(name), O_RDONLY);
>     cenforce(fd != -1, name);
>     scope(exit) std.c.linux.linux.close(fd);
>
>     struct_stat statbuf = void;
>     cenforce(std.c.linux.linux.fstat(fd, &statbuf) == 0, name);
>
>     immutable initialAlloc = statbuf.st_size ? statbuf.st_size + 1 : 1024;

Excellent idea, this allocates the minimum amount of space to tell if a file 
is larger than the file size returned in fstat.  This should handle the /sys 
case and /proc case properly.

>     void[] result = GC.malloc(initialAlloc, GC.BlkAttr.NO_SCAN)
>         [0 .. initialAlloc];
>     scope(failure) delete result;
>     size_t size = 0;
>
>     for (;;)
>     {
>         immutable actual = std.c.linux.linux.read(fd, result.ptr + size,
>                 result.length - size);
>         cenforce(actual != actual.max, name);
>         size += actual;
>         if (size < result.length) break;

Only bug:
You need to check if actual > 0.  A driver is allowed to return less data 
than requested, even if there is more to read.  You must read until read 
returns 0.  In this case you need a loop to do the reading.  Yes the disk 
driver never returns less than requested unless EOF is reached, but the 
/proc files are run by any driver, which might do something like that.

>         auto newAlloc = size + 1024 * 4;
>         result = GC.realloc(result.ptr, newAlloc, GC.BlkAttr.NO_SCAN)
>             [0 .. newAlloc];
>     }
>
>     return result[0 .. size];
> }

I would rewrite:

void[] read(string name)
{
    immutable fd = std.c.linux.linux.open(toStringz(name), O_RDONLY);
    cenforce(fd != -1, name);
    scope(exit) std.c.linux.linux.close(fd);

    struct_stat statbuf = void;
    cenforce(std.c.linux.linux.fstat(fd, &statbuf) == 0, name);

    auto allocSize = statbuf.st_size ? statbuf.st_size + 1 : 1024;
    void[] result;
    scope(failure) delete result;
    size_t size = 0;

outer:
    while(true)
    {
        result = GC.realloc(result.ptr, allocSize, GC.BlkAttr.NO_SCAN)
            [0 .. allocSize];
        while(size < allocSize)
        {
            immutable actual = std.c.linux.linux.read(fd, result.ptr + size,
                    allocSize - size);
            if(actual == 0)
                break outer;
            cenforce(actual != actual.max, name);
            size += actual;
        }
        allocSize = size + 1024 * 4;
    }

    return result[0 .. size];
}