Code dealing with files and separate lines in them

Question

I have been using similar piece of code since a while. I have read some issues regarding treating mapped memory as a string. I am not sure about that though, so I just use strndup(3) to avoid it anyway.

It's often said to use fopen, fwrite and all those f* functions over read(2), write(2) etc. The reason being, these functions handle the reading and writing quite well by intermediate buffering and are quite well organised too. But in my personal opinion, dealing with files with mmap(2) generally tends to be faster, gives nice control over the data because of MAP_* flags and making a structure yourself can organise data pretty well too. These things hold true except in some cases.

I wanted some reviews on this code and this way of working with files:

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

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct MyFile {
    const char *filename;
    int fd;
    size_t size;
    char *contents;
};

int setup_myfile(const char *filename, struct MyFile *file);
void clean_myfile(struct MyFile file);

int
main(int argc, char *argv[])
{
    if (argc != 2) {
        fprintf(stderr, "usage: %s filename\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    struct MyFile my_file;
    char *contents, *line, *to_free;

    int err = setup_myfile(argv[1], &my_file);

    if (err == -1) {
        fprintf(stderr, "setup_myfile() failed\n");
        exit(EXIT_FAILURE);
    }

    to_free = contents = strndup(my_file.contents, my_file.size);

    if (to_free == NULL) {
        perror("strndup(3)");
        exit(EXIT_FAILURE);
    }

    while ((line = strsep(&contents, "\n")) != NULL) {

        // Print line by line or do anything with line
        printf("%s", line);
    }

    free(to_free);
    clean_myfile(my_file);

    return 0;
}

int
setup_myfile(const char *filename, struct MyFile *file)
{
    struct stat st;

    file->filename = filename;
    file->fd = open(file->filename, O_RDONLY);

    if (file->fd == -1) {
        perror("open(2)");
        return -1;
    }

    if (fstat(file->fd, &st) == -1) {
       perror("fstat(2)");
       return -1;
    }

    file->size = st.st_size;    

    file->contents = mmap(NULL, file->size, PROT_READ | PROT_WRITE, MAP_PRIVATE, file->fd, 0);

    if (file->contents == MAP_FAILED) {
        perror("mmap(2)");
        return -1;
    }

    return 0;
}

void
clean_myfile(struct MyFile file)
{
    munmap(file.contents, file.size);
    close(file.fd);
}

Answer 1

the posted code does not cleanly compile!

Compile with warnings enabled, then fix those warnings.

for gcc, at a minimum use:

-Wall -Wextra -Wconversion -pedantic -std=gnu11

Note: other compilers use different options to produce the same results.

in function: setup_myfile() there are statements like:

return -1;

However, that only gets execution back to the call in main()

setup_myfile(argv[1], &my_file);

Which is ignoring the returned value. So, when setup_myfile() fails, main() will keep right on executing, as if everything is OK.

Suggest, rather than: return -1; to use:

exit( EXIT_FAILURE );

which will properly exit the program

regarding:

file->size = st.st_size;

This is performing an implicit conversion from off_t to unsigned long int. Probably better to declare file->size as an off_t.