3
\$\begingroup\$

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);
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1,Thanks for the suggestions. Those are certainly helpful but my main question is about dealing with files this way and mmap. \$\endgroup\$ – Mihir Oct 4 at 3:10
  • \$\begingroup\$ If you want discussion about using mmap() then should have posted to stackoverflow.com rather than here. 'Here' is for working programs that the OP would like to discover problems and/or improve \$\endgroup\$ – user3629249 Oct 5 at 0:53
  • \$\begingroup\$ New to this community, sorry for mistakes. In one way, its for improvement on how the code deals with files. There are various topics like performance differences, memory leak chances, security, code clarity etc etc. I will update my question to add more details. Thanks. \$\endgroup\$ – Mihir Oct 5 at 4:45

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.