Reading a file into memory

Question

Can you review below code for performance and stability considerations? It is supposed to take a filename or NULL. If it is a filename, read given file into memory. If it is NULL, read from stdin into memory.

struct file_data {
    char *data;
    size_t numChars;
};

/* Read whole file into memory */
struct file_data read_file(char *filename) {
    FILE *f;
    char buffer[BUFFER_SIZE];
    char *data = NULL;
    size_t current_size = 0;
    size_t totalChars = 0;
    size_t nchars;

    if(filename) {
        f = fopen(filename, "r");
    } else {
        f = stdin;
    }

    do {
        data = (char *)realloc(data, current_size + BUFFER_SIZE);
        assert(data);
        current_size+=BUFFER_SIZE;
        nchars = fread(buffer, 1, BUFFER_SIZE, f);
        memcpy(&data[totalChars], buffer, nchars);
        totalChars+=nchars;
    } while(nchars == BUFFER_SIZE);

    struct file_data fd = {data, totalChars};
    return fd;
}

Answer 1

Check return values

You're not making sure fopen isn't failing when opening a file:

f = fopen(filename, "r");

Also check the return value of fread for failure:

The total number of elements successfully read is returned. If this number differs from the count parameter, either a reading error occurred or the end-of-file was reached while reading. In both cases, the proper indicator is set, which can be checked with ferror and feof, respectively. If either size or count is zero, the function returns zero and both the stream state and the content pointed by ptr remain unchanged. size_t is an unsigned integral type.

const parameters

The function should never modify the contents of the string filename, so declaring the parameter const char *filename is good practice.

How does read_file signal a failure

You're asserting on realloc failure. I usually prefer returning gracefully even on alloc failures and leave it open to the caller to be able to cope and at least shutdown nicely. But that is not necessary in all cases.

However, touching the same area is how the read_file function signals a failure to it's caller? One way would be:

struct file_data fd = {NULL, 0};
return fd;

Or changing the function to:

int read_file(const char *filename, struct file_data *fd)
{
     assert(fd);
     ...
     fd->data = data;
     fd->numChars = totalChars;
     return 0; // On success.
     ...
     return -1; // On failure.
}

Read directly into the data buffer

You're doing unnecessary copying by using memcpy and a separate temporary buffer to read the file contents into.

How you could read it into the data buffer directly instead (getting rid of buffer):

size_t offset = 0;
nchars = fread(&data[offset], 1, BUFFER_SIZE, f);
// TODO: Remember to check nchars for error condition.
offset += nchars;

Answer 2

If you want to read the entire contents of a file into a buffer in memory, then the best way to do it is to map the file into memory, using mmap on Unix, or CreateFileMapping and MapViewOfFile on Windows. This has a bunch of advantages over the approach in the post:

1. It's fast: the memory copy (in the fread) is avoided.

2. It's lazy: the memory gets mapped as soon as mmap returns, but does not necessarily get read from disk until the process actually touches the memory. So if your program doesn't use the whole file, then only the parts it does use are loaded.

3. The mapping operation succeeds or fails as a whole: you don't have to deal with error cases in which you've loaded part of the file into memory.

The downsides of memory-mapped I/O are:

1. It's not portable.

2. It only works on disk files (not on sockets, pipes, terminals, etc) and so it's less general than standard I/O.

Answer 3

I have a problem with the line

    data = (char *)realloc(data, current_size + BUFFER_SIZE);

The problem is that this realloc()s each time through the loop, increasing by BUFFER_SIZE each time. It would be more efficient for each realloc() double the size of data, and only do so when necessary, instead of each time through the loop.