Minimalistic rmdir

Question

In my eyes most of the GNU stuff is bloated and doesn't really fit my view of how a Linux environment should look like. I have come up with my own minimalist implementation of the GNU Coreutils, but I have not touched C in a long time and my programming style reflects that a little.

rmdir.c is a good representation of it throughout the project:

#include <sys/stat.h>
#include <sys/types.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <getopt.h>
#include <errno.h>
#include <string.h>
void process_path(char *path, bool verbose, bool parents);
int main(int argc, char** args){
    bool verbose = false;
    bool parents = false;

    char c;
    while ((c = getopt(argc, args, "vpr")) != -1){
        switch(c){
            case 'r':
            case 'p': parents = true; break;
            case 'v': verbose = true; break;
            default: return -1;
        }
    }

    /* Proccess each path in the remaining arguments */
    int index;
    for (index = optind; index < argc; index++){
        process_path(args[index], verbose, parents);
    }

    return 0;
}

void remove_dir(char *path, bool verbose){
    int ret = rmdir(path);
    if(verbose){
        if(ret == -1) printf("rmdir: could not remove '%s' %s \n", path, strerror(errno));
        if(ret ==  0) printf("rmdir: removed directory '%s' \n", path);
    }
}

void do_parents(char *path, bool verbose){
    while(1) {
        remove_dir(path, verbose);
        char *p = strrchr(path, '/');
        if(p == NULL) break; 
        *p = '\0';
    }
}

void process_path(char *path, bool verbose, bool parents){
    switch(parents){
        case false: remove_dir(path, verbose); break;
        case  true: do_parents(path, verbose); break;
    }
}

Statically linked against musl-libc, the executable is only 18KiB, as opposed to the GNU version 39KiB.

Any ideas on how I can improve the coding style or make things even smaller?

Answer 1

Style

Minor nitpicking: your style is not 100% consistent.

int main(int argc, char** args){
// should be
int main(int argc, char **args){

while(1) {
// should be
while(1){

Pattern

I think this block is very, very unusual.

switch(parents){
    case false: remove_dir(path, verbose); break;
    case  true: do_parents(path, verbose); break;
}

A switch with a bool is the same as:

if(parents){
    do_parents(path, verbose);
} else {
    remove_dir(path, verbose);
}

Memory manipulation

void do_parents(char *path, bool verbose){
    while(1) {
        remove_dir(path, verbose);
        char *p = strrchr(path, '/');
        if(p == NULL) break; 
        *p = '\0';
    }
}

I think this is nasty: it modifies the string pointed to by path. Granted, it is a char * instead of const char *, but ultimately you're modifying the string from your mains args (BTW, the convention is to call that argv). I would make a local copy of path for that.

That way, you could turn almost all your char * into const char * (and const is good since it helps find accidental manipulation bugs).

Standard compliance

What is your -r option? Neither my GNU nor my BSD rmdir know it. Since you fall through to -p they're equivalent so you might be thinking recursive? It needs a comment, IMHO.

Help

If your tool is called with wrong or missing argument, it silently exits. IMHO a good command line tool should always have a help/syntax page.

Return value from main

When you encounter an unsupported option you do:

default: return -1;

You shouldn't return a negative value from main as for POSIX environments, the return (exit) value is just an 8-bit unsigned integer. Values above 125 or 128 are usually treated specially by the shell (marking that it was terminated by a signal). But -1 in two's complement limited to 8 bits is 1111 1111 = 255. Thus, your shell might think your command got killed.

So instead of -1 you should return 1. But actually, according to C99, the correct values to use are return EXIT_FAILURE; in your switch and return EXIT_SUCCESS; at the end of your main.

Personal opinion

This is just my personal opinion and I know others might not disagree, but here goes: I think an if should always have curly braces, except if its body is short and on the same line.

So I think this is OK:

if(p == NULL) break;

But I think this:

if(ret == -1) printf("rmdir: could not remove '%s' %s \n", path, strerror(errno));

is too long and thus should be broken into several lines:

if(ret == -1){
    printf("rmdir: could not remove '%s' %s \n", path, strerror(errno));
}

I think it's a not a good idea to have an if and a newline without curly braces: either it's one line or it has to have braces. For reasons, see Apple's goto fail;.

Logic/intention

This is also more on the personal opinion side:

if(ret == -1) printf("rmdir: could not remove '%s' %s \n", path, strerror(errno));
if(ret ==  0) printf("rmdir: removed directory '%s' \n", path);

Apart from the fact that I think these ifs should be broken into several lines, I think they should be written as:

if(ret == -1){
   printf("rmdir: could not remove '%s' %s \n", path, strerror(errno));
} else {
   printf("rmdir: removed directory '%s' \n", path);
}

Either the call to rmdir succeeds or it doesn't. Your version doesn't make it clear that this really is an either/or.

Answer 2

As remove_dir() returns void, you are discarding the return status of rmdir(). If the verbose flag is not enabled, the program will succeed or fail silently, with no way to distinguish the two other than inspecting the directories that exist afterwards. A failure ought to result in the immediate termination of recursion and a non-zero exit status. If there are messages, I would expect them to go to stderr rather than stdout.