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\$
  • 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\$ – Harald Scheirich Jan 24 '18 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. Jan 24 '18 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 Jan 24 '18 at 18:46
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\$
  • \$\begingroup\$ Thanks a lot for those comments, which totally make sense. \$\endgroup\$ – Thibaut D. Jan 24 '18 at 20:10

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.