Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
3  
I need to ask: why? Do you do it for fun/learning? Do you need it for an embedded/space constraint environment and think you can do better than Busybox or Toybox? I don't want to discourage you or anything, just want to know why you're trying to re-implement something that already has been re-implemented many times. –  DarkDust Aug 30 at 20:52
    
I wouldn't call 39KiB bloated... Not on a computer that typically has at least 262144000 KiB of storage. –  Emily L. Aug 30 at 21:58
    
In pursuit of purity, you ought to collapse rmdir into the rm utility, as in Plan9. (C source here.) The same program was ported to Unix-likes in Plan 9 from User Space. (That source is here.) –  Anko Aug 30 at 22:12
    
@EmilyL. He didn't say that rmdir itself is bloated, but "most of the GNU stuff". But on an embedded system where you sometimes measure the available space is mega- or even kilobytes, 39KiB are quite a lot for this simple tool. Even 18KiB is way more than would be really needed. For comparison: I once did a minimal mount with optional -t support for an embedded Linux system in assembler: executable is 1152 bytes, help and six error messages included. –  DarkDust Aug 30 at 22:43

2 Answers 2

up vote 11 down vote accepted

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.

share|improve this answer
1  
Can you please cite this? - "You shouldn't return a negative value from main as for POSIX environments" –  syb0rg Aug 30 at 22:01
2  
@syb0rg: See Wikipedia "Exit status". It's a consequence of to the WEXITSTATUS macro. See also the POSIX/SUS page for wait. –  DarkDust Aug 30 at 22:13
1  
Small correction: EXIT_SUCCESS and EXIT_FAILURE were introduced in C89, not C99. And if you're rewriting the GNU coreutils, you probably do want to stick to C89. :-) –  Thomas Aug 30 at 23:33

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.

share|improve this answer
    
+1 on return value and stderr. But since he's using strerror he would already get meaningful error messages and using perror instead thus isn't necessary, IMHO. But might be the better choice nevertheless. –  DarkDust Aug 31 at 7:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.