4
\$\begingroup\$

How does the following look to list the directories on a posix system? This is actually the first piece of C code that I've written where I've actually been able to use for a real-world problem!

#define _GNU_SOURCE
#include <dirent.h>
#include <string.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/syscall.h>

#define handle_error(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)

struct linux_dirent {
    unsigned long  d_ino;
    off_t          d_off;
    unsigned short d_reclen;
    char           d_name[];
};

void print_files(char* dir, FILE* out)
{
    // open the file
    int fd = open(dir, O_RDONLY | O_DIRECTORY);
    if (fd == -1) handle_error("Error opening file.\n");

    // grab a buffer to read the file data
    #define BUF_SIZE (1024*1024*1)
    char* buffer = malloc(sizeof *buffer * BUF_SIZE);
    if (buffer == NULL) handle_error("Error malloc.\n");

    // do the getdents syscall writing to buffer
    int num_read = syscall(SYS_getdents, fd, buffer, BUF_SIZE);
    if (num_read == -1) handle_error("Error getdents syscall.\n");
    close(fd);

    for (long buffer_position = 0; buffer_position < num_read;) {

        struct linux_dirent *d = (struct linux_dirent *) (buffer + buffer_position);
        char d_type = *(buffer + buffer_position + d->d_reclen - 1);

        // skip on . and .. in the listing
        if (d->d_name[0] == '.') {
            buffer_position += d->d_reclen;
            continue;
        }

        // path = dir + '/' + name
        char path[400];
        strcpy(path, dir);
        strcat(path, "/");
        strcat(path, d->d_name);

        // recursive call, as necessary
        if (d_type == DT_DIR)
             print_files(path, out);
        else if (d_type == DT_REG)
            fprintf(out, "%s\n", path);

        // advance buffer position
        buffer_position += d->d_reclen;

    }

    free(buffer);

}

int main(int argc, char *argv[])
{
    char dir[1024];
    strcpy(dir, argc > 1 ? argv[1] : ".");
    FILE *out = fopen("c-log.txt", "w");
    fprintf(out, "-------------[ START ]---------------------\n");
    print_files(dir, out);
}
```
\$\endgroup\$
5
\$\begingroup\$
#define handle_error(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)

There's a long-standing convention to use UPPER_CASE for preprocessor macros. This helps draw attention to them as text replacements, which don't obey the same rules of scope as C identifiers (and which sometimes - though not in this case - expand their arguments multiple times).

void print_files(char* dir, FILE* out)

Why a const char* dir? Surely we have no need to overwrite its contents? Also, I'd be inclined to always write to stdout in this program - let the user apply ordinary shell plumbing if a different destination is desired. That's easier than having a special argument in each program.

In larger programs, it's usually a bad idea to exit() from within functions, particularly if it's possible to either recover, or produce useful information regardless of the error. As a concrete example, if we find a subdirectory we can't read, it's probably more helpful to emit a warning message to stderr but continue listing the directories the user does have permissions for.

The code using open() and syscall() is much lower level than we'd normally use for this, and tightly bound to Linux rather than POSIX as claimed. I think opendir() and readdir() would be more suited.

Be careful with resource management and non-local exits in C. We only reach close(fd) in the success case. Although handle_error() currently exits the program, if we change that in future, we'll find ourselves leaking file descriptors. Kudos for closing the descriptor before the recursive call - it's easy to leave it open until the end of the function, and you avoided that trap.

char* buffer = malloc(sizeof *buffer * BUF_SIZE);

The multiplication by sizeof *buffer is borderline pointless. I say "borderline" in case there's a chance that we might want to change the type (but still have BUF_SIZE of them).

We don't seem to handle reading directories larger than BUF_SIZE at all - not even with an error message.

    char path[400];
    strcpy(path, dir);
    strcat(path, "/");
    strcat(path, d->d_name);

Where's that 400 come from? Why not use the system's MAX_PATH constant?

strcpy() followed by strcat() is wasteful - that causes another traversal of the string just written, in order to find where to start writing. In this case, we could use printf() like this:

int path_len = snprintf(path, sizeof path, "%s/%s", dir, d->d_name);

Remember to test path_len to determine whether the path was truncated at this stage.

    if (d_type == DT_DIR)
         print_files(path, out);
    else if (d_type == DT_REG)
        fprintf(out, "%s\n", path);

Why don't we print the other kinds of file? I wouldn't expect all my symlinks, sockets, FIFOs and device files to simply disappear from the listing unless I specifically asked for that.

   fprintf(out, "-------------[ START ]---------------------\n");

Consider using fputs() when there's no conversions to be done.

As a general style point, I'd advise more vertical space for conditionals, and use of braces as a consistent rule. E.g.

if (buffer == NULL) {
    handle_error("Error malloc.\n");
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ While sizeof is redundant when the type is char, I think it's a reasonable habit to get into always writing type *var = malloc(sizeof *var * count);. And little clarity is lost in using printf() with no conversions (the compiler will almost certainly optimize it, so no performance is lost either). \$\endgroup\$ – Barmar Apr 22 at 15:29
  • \$\begingroup\$ Yes, to both of those points. \$\endgroup\$ – Toby Speight Apr 22 at 15:35
  • \$\begingroup\$ " use UPPER_CASE for preprocessor macros." is interesting in that it was followed shortly by FILE, a type that may or may not be defined via a macro. \$\endgroup\$ – chux - Reinstate Monica Apr 22 at 17:32
  • 1
    \$\begingroup\$ "Consider using fputs() when there's no conversions to be done." --> is OK, but borders on premature optimization. Good compilers emit efficient code with fprintf(out, "[ START ]\n"); or fputs( "[ START ]\n", out);. For such minor issues, best to follow groups coding standard and/or clarity. \$\endgroup\$ – chux - Reinstate Monica Apr 22 at 17:38

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.