Recursively delete directory using relative filesystem calls

Question

I made this function to recursively delete a directory and its contents, because using VLAs to concatenate paths is more susceptible to Stack Overflow and is probably slower.

#include <unistd.h>
#include <dirent.h>
#include <fcntl.h>
#include <string.h>
int SavesFD;
static int Recursive(const int File, const char *const Name) {
    const int File0 = openat(File, Name, O_DIRECTORY);
    if (File0 == -1)
        return 1;
    DIR *const Dir = fdopendir(File0);
    if (!Dir)
        return 1;
    for (struct dirent *Dirent; (Dirent = readdir(Dir));)
        if ((*Dirent->d_name != '.' || (Dirent->d_name[1] && *(uint16_t *)(Dirent->d_name+1) != '.')) && (Dirent->d_type == DT_DIR ? Recursive(File0, Dirent->d_name) : unlinkat(File0, Dirent->d_name, 0)))
            return 1;
    return closedir(Dir) || unlinkat(File, Name, AT_REMOVEDIR);
}
void DeleteSave(const char *const Name) {
    if (Recursive(SavesFD, Name))
        perror(Name);
}

I believe my code is efficient but not very readable or easy to comprehend; is this an example of premature micro-optimization?

And also, if I am repeatedly accessing the value of Dirent->d_name or Dirent->d_name[1] should I consider storing these in variables?

Answer 1

There's no need for SavesFD to be a global - just pass it as a parameter.

Why does DeleteSave() do its own error reporting? It would be more useful if it simply returned a status for the calling code to take appropriate action.

Naming: Recursive tells us only how the function is implemented, but we really need to know what it does.

The reinterpret-cast of Dirent->d_name+1 to uint16_t* is non-portable. It probably does what you wanted on little-endian hardware (if it has lax alignment constraints), but almost certainly not otherwise.

The test inside the loop is very unreadable as one long line. It could be clearer as more if statements - perhaps like

struct dirent *d;
while ((d = readdir(dir))) {
    if (!strcmp(".", d->d_name)) { continue; }
    if (!strcmp("..", d->d_name)) { continue; }
    if (d->d_type == DT_DIR) {
        if (delete_dir(fd, d->d_name)) { return 1; }
    } else {
        if (unlinkat(fd, d->d_name, 0)) { return 1; }
    }
}

We shouldn't care if unlinkat() fails because some other process removed the entry before we could get to it, because the desired result has been achieved.

---

Modified code

#ifndef DELETE_DIR
#define DELETE_DIR

int delete_dir(const int fd, const char *name);

#endif

#define _POSIX_C_SOURCE 200809L
#define _DEFAULT_SOURCE

#include "delete_dir.h"

#include <dirent.h>
#include <fcntl.h>
#include <unistd.h>

#include <errno.h>
#include <string.h>

int delete_dir(const int fd, const char *const name)
{
    int dir_fd = openat(fd, name, O_RDWR|O_NOFOLLOW|O_DIRECTORY);
    if (dir_fd < 0) { return 1; }

    DIR *dir = fdopendir(fd);
    if (dir) { return 1; }

    struct dirent *d;
    while ((d = readdir(dir))) {
        if (d->d_type == DT_DIR) {
            if (!strcmp(".", d->d_name)) { continue; }
            if (!strcmp("..", d->d_name)) { continue; }
            if (delete_dir(fd, d->d_name) && errno != ENOENT) { return 1; }
            if (unlinkat(fd, d->d_name, AT_REMOVEDIR)
                && errno != ENOENT) { return 1; }
        } else {
            if (unlinkat(fd, d->d_name, 0) && errno != ENOENT) { return 1; }
        }
    }
    return closedir(dir);
}