1
\$\begingroup\$

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?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Did you test it with file/directory names like ..foo (that is starting with 2 or more dots)? \$\endgroup\$
    – vnp
    Jan 24, 2022 at 1:52
  • \$\begingroup\$ @vnp I do not see why it would not work \$\endgroup\$
    – user254467
    Jan 24, 2022 at 2:19

1 Answer 1

1
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.