patterncModerate
Minimalistic rmdir
Viewed 0 times
minimalisticrmdirstackoverflow
Problem
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:
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?
rmdir.c is a good representation of it throughout the project:
#include
#include
#include
#include
#include
#include
#include
#include
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?
Solution
Style
Minor nitpicking: your style is not 100% consistent.
Pattern
I think this block is very, very unusual.
A
Memory manipulation
I think this is nasty: it modifies the string pointed to by
That way, you could turn almost all your
Standard compliance
What is your
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
When you encounter an unsupported option you do:
You shouldn't return a negative value from
So instead of
Personal opinion
This is just my personal opinion and I know others might not disagree, but here goes: I think an
So I think this is OK:
But I think this:
is too long and thus should be broken into several lines:
I think it's a not a good idea to have an
Logic/intention
This is also more on the personal opinion side:
Apart from the fact that I think these
Either the call to
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
mainWhen 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.Code Snippets
int main(int argc, char** args){
// should be
int main(int argc, char **args){
while(1) {
// should be
while(1){switch(parents){
case false: remove_dir(path, verbose); break;
case true: do_parents(path, verbose); break;
}if(parents){
do_parents(path, verbose);
} else {
remove_dir(path, verbose);
}void do_parents(char *path, bool verbose){
while(1) {
remove_dir(path, verbose);
char *p = strrchr(path, '/');
if(p == NULL) break;
*p = '\0';
}
}default: return -1;Context
StackExchange Code Review Q#61582, answer score: 11
Revisions (0)
No revisions yet.