15
\$\begingroup\$

Here I have a method for reading the contents of a file into a buffer and returning whether or not it is successful.

Here is what I would like reviewed:

  • Speed: Am I reading in the file as fast as possible?

  • Bugs: Are there cases where my code could fail or give unexpected behavior?

  • Syntax/Styling: How does my code look? How could I make it look better? Anything I'm doing wrong syntax-wise?


int getFileContents(const char *file, void **contents, size_t *size)
{
    // Open file for reading
    int fd = open(file, O_RDONLY);
    if (fd < 0) return -1;

    off_t total = lseek(fd, 0, SEEK_END);
    if (total < 0) return -1;

    lseek(fd, 0, SEEK_SET);

    ssize_t n, rest = total, readb = 0;
    unsigned char buf[BUF_SIZE];
    unsigned char *result = NULL;

    // Read the whole file into memory
    while (rest > 0) {
        n = read(fd, buf, BUF_SIZE);
        if (n < 0) {
            close(fd);
            free(result);
            return -1;
        }

        unsigned char *tmp = (unsigned char*) realloc(result, readb + n);
        if (!tmp) {
            close(fd);
            free(result);
            return -1;
        }

        result = tmp;
        memcpy(result + readb, buf, n);
        readb += n;
        rest -= n;
    }

    close(fd);
    *contents = result;
    *size = total;

    return 0;
}
\$\endgroup\$
0
21
\$\begingroup\$

1. Bugs

  1. If the open succeeds but the lseek fails (as it might if the file is a named pipe), the function omits to close the file descriptor, thus leaking it.

  2. If read reaches the end of the file (as it might if the file was truncated after the lseek), then it returns 0. This will cause the function to go into an infinite loop.

  3. There's no check on the result of the second call to lseek. If this call fails, then the function will go into the infinite loop described above.

2. Review

  1. There's no specification. What does this function do? How do I call it? What does it return?

  2. This is a worst-case \$\Theta(n^2)\$ algorithm, because realloc may have to copy the result each time it grows, and you grow the buffer by at most BUF_SIZE bytes each time you call realloc. It's usual in this kind of situation to grow the result exponentially, by multiplying the result size by some constant on each realloc call.

  3. Even ignoring the copying due to realloc, each byte read from the file is copied again using memcpy. It would be better to realloc the result first, and then read directly into the result.

  4. You go to the trouble of finding the length of the file, so why not read it all in one go?

    result = malloc(total);
    if (!result)
        /* handle error */
    n = read(fd, result, total);
    if (n != total)
        /* handle error */
    
  5. Consider using fstat instead of lseek to determine the size of the file.

  6. It's not a good idea to put multiple statements on the same line, like this:

    if (fd < 0) return -1;
    

    That's because in most debuggers you won't be able to set a breakpoint on the return statement.

  7. There's repetition in the handling of the four failure cases. This makes the function difficult to modify (because you might fail to adjust one or more of the cases). The bug in §1.1 above might have been caused by this problem.

  8. For reading the entire contents of a file into memory, consider using mmap.

    mmap has advantages over read. (i) There's no need to allocate a buffer: the kernel maps the file directly into the process's virtual memory. (ii) mmap operates lazily: it maps the file into memory but does not actually copy any part of it until that page is touched, so programs that randomly access the file don't have to wait for the whole file to load.

    mmap also has disadvantages. (i) mmap only operates on ordinary files (not pipes, sockets, or other kinds of file descriptor). (ii) Mapping lots of files creates lots of entries in the process's virtual memory table, and on some operating systems (notably OS X) this can lead to poor memory performance.

3. Revised code

#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <unistd.h>

/* Map the contents of file into memory: if successful, update
 * *contents to point to the file contents, update *size to be the
 * size of the file, and return 0. Otherwise, return -1.
 */
int getFileContents(const char *file, void **contents, size_t *size)
{
    int fd = open(file, O_RDONLY);
    if (fd < 0)
        goto fail_open;

    struct stat st;
    if (fstat(fd, &st) < 0)
        goto fail_fstat;

    void *result = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
    if (result == MAP_FAILED)
        goto fail_mmap;

    *contents = result;
    *size = st.st_size;
    close(fd);
    return 0;

fail_mmap:
fail_fstat:
    close(fd);
fail_open:
    return -1;
}       

4. Answers to comments

In comments, lesto suggests:

  1. I would return a different number for every error, to help the user of the function to identify the problem and try to recover.

    In this context (Unix systems programming), it's conventional for a function to return a single distinguished value (typically −1) to indicate an error, with the cause of the error being stored in errno. See for example the POSIX specification of open:

    Upon successful completion, these functions shall open the file and return a non-negative integer representing the lowest numbered unused file descriptor. Otherwise, these functions shall return -1 and set errno to indicate the error.

    It makes sense to stick to the Unix convention, so that the caller can apply the same kind of error handling to getFileContents as they do to other system interfaces.

  2. Every error between open and close need the call to close, and leaving it inline is easier than check the list of label is correct, IMHO. It is true that you are writing some more lines.

    If you write out the error-handling code inline, then your functions have length that's quadratic in the number of error cases! This is disastrous for readability and maintainability. It is all too easy to forget one of the cases, as in the bug described in §1.1 above.

    An alternative to goto is to use nested if statements. In this case the code would become:

    int fd = open(file, O_RDONLY);
    if (fd >= 0) {
        struct stat st;
        if (fstat(fd, &st) == 0) {
            void *result = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
            if (result != MAP_FAILED) {
                *contents = result;
                *size = st.st_size;
                close(fd);
                return 0;
            }
        }
        close(fd);
    }
    return -1;
    

    but this quickly becomes hard to read because of the indentation.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Excellent review, nice work! \$\endgroup\$
    – Mast
    May 25 '15 at 19:18
  • \$\begingroup\$ instead of goto i would return a different number for every error, to help the user of teh function to identify the problem and try to recover. Every error between "open" and "close" need the call to close(), and leaving it inline is easier than check the list of label is correct, IMHO. It is true that you are writing some more line \$\endgroup\$
    – Lesto
    May 26 '15 at 0:48
  • \$\begingroup\$ @lesto: Replies in §4. \$\endgroup\$ May 26 '15 at 9:44
  • 1
    \$\begingroup\$ @GarethRees now you have my upvote. Thank your for clarifications \$\endgroup\$
    – Lesto
    May 26 '15 at 10:05
7
\$\begingroup\$

Since you get the total size of the file when you do total = lseek(fd, 0, SEEK_END), I don't see the point in reading in BUF_SIZE chunks. You could read the whole thing in one go, and greatly simplify your code, not to mention the reduced memory copies. Something like this:

int getFileContents(const char *file, void **contents, size_t *size)
{
    int fd = open(file, O_RDONLY);
    if (fd < 0) return -1;

    off_t total = lseek(fd, 0, SEEK_END);
    if (total < 0) return -1;

    lseek(fd, 0, SEEK_SET);

    ssize_t n;
    unsigned char *result = (unsigned char*) malloc(total);

    n = read(fd, result, total);
    if (n < 0) {
        close(fd);
        free(result);
        return -1;
    }

    close(fd);
    *contents = result;
    *size = total;

    return 0;
}

And to answer your questions:

Speed: Am I reading in the file as fast as possible?

I think my suggestion is faster, thanks mainly to the reduced allocations and memory copies.

Bugs: Are there cases where my code could fail or give unexpected behavior?

I cannot see.

Syntax/Styling: How does my code look? What about it could make it look better? Anything I'm doing wrong syntax wise?

The comment // Open file for reading right before the open call is really unnecessary. Other than that, it seems fine.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ read can read less than what you requested without it being an error. Your version doesn't account for that, potentially returning a partially-filled buffer to the caller. \$\endgroup\$
    – Mat
    May 26 '15 at 7:07
  • \$\begingroup\$ Yeah, but that's not a bug I introduced, it was already there in OP's code. Gareth treated it already, and better, so I won't fix that now. \$\endgroup\$
    – janos
    May 31 '15 at 16:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.