2
\$\begingroup\$

I wrote a function to read the entire content of a file and return a pointer to an allocated bufer containing all the data.

Please could you tell me if there is a better way of doing this ? One solution could be to use stat() to pre-allocate a buffer and do only one read() call but I don't know if it's better, any though ?

#include <stdio.h>
#include <memory.h>

char *file_read(const char *path, size_t *file_size)
{
    FILE *file = fopen(path, "r");
    char buffer[FILE_READ_BUFFER_SIZE];
    char *tmp = NULL;
    char *data = NULL; // The buffer containing the data of the file
    char *data_cursor = NULL; // The current position in the data buffer
    size_t data_size = 0; // The size of the data buffer
    size_t n = 0; // The number of bytes read by fread()

    if (file == NULL)
        goto error;

    while ((n = fread(buffer, sizeof(char), sizeof(buffer), file)) > 0) {
        if (data == NULL) {
            data = malloc(sizeof(char) * n);
            data_cursor = data;
            data_size = n;
        } else {
            // We compute the position in the buffer as an integer
            // to be able to re-apply after realloc(). Because realloc()
            // can move the pointer we need this position as an integer.
            size_t cursor_pos = data_cursor - data;
            tmp = realloc(data, sizeof(char) * data_size + n);
            if (tmp == NULL)
                goto error;
            data = tmp;
            data_cursor = data + cursor_pos;
            data_size += n;
        }
        memcpy(data_cursor, buffer, n);
    }

    if (ferror(file))
        goto error;

    fclose(file);
    if (file_size != NULL)
        *file_size = data_size;
    return data;

    error:
    if (file != NULL)
        fclose(file);
    if (data != NULL)
        free(data);
    return NULL;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Code posted to codereview should compile, it would be great if you could make this a complete file especially could you post the code for error_set_from_errno_ptr() ? \$\endgroup\$ Commented Jan 24, 2018 at 13:45
  • \$\begingroup\$ It's just a utility function which stores internally the last errrno & strerror() and return NULL. I replaced it with return NULL in my example. Thanks ! \$\endgroup\$
    – Thibaut D.
    Commented Jan 24, 2018 at 14:48
  • \$\begingroup\$ Here you find how to find size of a file using fseek+ftell, and then allocating a buffer that exact size: stackoverflow.com/questions/5957845/… After that use rewind and fread. \$\endgroup\$
    – Andreas
    Commented Jan 24, 2018 at 18:46

1 Answer 1

2
\$\begingroup\$
  • I recommend to pass a file pointer rather than a path. This will enable the code to work with streams as well. It would also simplify the awkward error handling.

  • Modern allows, and recommends, to declare variables closer to their use.

  • It is OK to realloc even when data is NULL.

  • sizeof(char) is guaranteed to be 1.

  • Incrementing data_size after memcpy lets you drop data_cursor and cursor_pos, which IMHO obfuscate the code. You'd need to

        memcpy(data + data_size, buffer, n);
        data_size += n;
    
  • Allocating an array on the stack is usually not recommended. Consider reading directly into data + data_size, and get away without buffer and memcpy.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for those comments, which totally make sense. \$\endgroup\$
    – Thibaut D.
    Commented Jan 24, 2018 at 20:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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