Opening a directory as a file descriptor creating it if it does not exist [closed]

Question

Variables:

• conf of type struct Conf whose details are not relevant to this review stores the configuration data being read.
• str is a string containing the path of the directory the app uses to save data.

Code:

bool exist = 0;
if ((mkdir(str, 0755) && !(exist = errno == EEXIST)) || (Dir = open(str, O_DIRECTORY)) == -1) {
    perror(str);
    return errno;
}
if (exist) {
    const int fd = openat(Dir, ".Conf", O_RDONLY);
    if (fd != -1) {
        const ssize_t r = read(fd, &conf, sizeof(struct Conf));
        if (r < sizeof(struct Conf))
            r != -1 ? (void)memset(&conf, 0, r) : perror(".Conf");
        if (close(fd))
            perror(".Conf");
    } else if (errno != ENOENT)
        perror(".Conf");
}

The issue is I am not sure if this is the cleanest way to accomplish this task. The program relies on the mkdir system call failing to signal that the directory already exists. Is it good practice to expect a system call to fail?

Another issue is creating a bool variable to set somewhere and use as a condition elsewhere, was what I thought of as a quick hack to achieve the correct logic.

What is a cleaner way to achieve this?

Answer 1

I had to add a lot of declarations to get this anywhere near compilable:

struct Conf
{
    char a;
    unsigned char b;
};

#define _POSIX_C_SOURCE 200809L

#include <stdbool.h>
#include <stdio.h>
#include <errno.h>
#include <sys/stat.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>

int read_conf(const char *str, struct Conf conf)
{
    int Dir;

Beware of reading from file directly into a structure like this - any small change to structure layout (e.g. using different a compiler) will cause data corruption.

---

This line is hard to read, with its mix of conditionals and side-effects:

if ((mkdir(str, 0755) && !(exist = errno == EEXIST)) || (Dir = open(str, O_DIRECTORY)) == -1) {

I recommend encapsulating this in a function that is similar to open(str, O_CREAT) (but creating a directory rather than a plain file). Even if it's only used once, it makes the code much clearer to understand.

In any case, we need to close Dir once we're finished with it, so we're leaking the descriptor.

---

The if (exist) test seems pointless, as if the directory is newly-created, openat() will quickly fail with ENOENT. Saving a single system-call is unlikely to provide a measurable benefit, so go with the simpler code.

It's usually simpler to deal with error cases first - if we can return early, that reduces indentation in subsequent code, and therefore reduces cognitive load.

---

There's some more confusing code:

        if (r < sizeof(struct Conf))
            r != -1 ? (void)memset(conf, 0, r) : perror(".Conf");

Much clearer if we unwrap that:

    if (r < 0) {
        perror(".Conf");
        goto err;
    }
    const size_t ur = (size_t)r;
    if (ur < sizeof conf) {
        memset(&conf, 0, ur);
        goto err;
    }

    ⋮

    int retval;
 err:
    retval = errno;
    close(fd);
    return retval;

It still doesn't make sense - why do we overwrite just the part the config we read? And is all-bytes-zero appropriate for a struct Conf?

We should be continuing the read, like this:

static bool read_n(int fd, char *dest, size_t count)
{
    while (count) {
        ssize_t bytes_read = read(fd, dest, count);
        if (bytes_read <= 0) {
            return false;
        }
        dest += bytes_read;
        count -= (size_t)bytes_read;
    }
    return true;
}

And probably better to keep a static const struct Conf default_config around, for the purpose of overwriting.

---

Passing a struct Conf by value means that we are modifying the wrong structure. Pass it as a pointer, so we can write to the caller's version.

---

Naming is inconsistent, with a mix of lower-case and PascalCase names. Stick with one convention (preferably the platform's usual convention) to avoid making life difficult for maintainers.

Names such as str carry no useful information. Prefer names that convey the purpose of variables, rather than their type.

Since exist is a boolean, prefer to initialise with false rather than 0.

Prefer to use sizeof on objects rather than types. That makes it easier to see that the correct size is being used in calls such as read(fd, &conf, sizeof conf).

---

Improved code

static int open_or_mkdir(const char *dirname)
{
    /* try creating it (harmless if it already exists) */
    mkdir(dirname, 0755);
    /* open could still fail, if mkdir failed or another process
       intervenes */
    return open(dirname, O_DIRECTORY);
}

static bool read_n(int fd, void *dest, size_t count)
{
    char *p = dest;
    while (count) {
        ssize_t bytes_read = read(fd, p, count);
        if (bytes_read == 0) {
            /* this will have to do to indicate file-too-short */
            errno = EPIPE;
        }
        if (bytes_read <= 0) {
            return false;
        }
        p += bytes_read;
        count -= (size_t)bytes_read;
    }
    return true;
}

int read_conf(const char *str, struct Conf *conf)
{
    int dir = open_or_mkdir(str);
    if (dir < 0) {
        perror(str);
        return errno;
    }

    const int fd = openat(dir, ".Conf", O_RDONLY);
    close(dir);
    if (fd < 0) {
        if (errno == ENOENT) {
            /* fine, and normal */
            return 0;
        }
        perror(".Conf");
        return errno;
    }

    errno = 0;
    read_n(fd, conf, sizeof *conf);
    if (close(fd)) {
        perror(".Conf");
    }
    return errno;
}