Recovery Shell (rsh)

Realloc strategy

I'm not a fan of how you are reallocing your token array to be one element bigger on each new token. That turns into an \$O(N^2)\$ task. I would recommend either:

1. Do one quick pass over the input to count the number of tokens.
2. Doubling the size of the token array each time.

Another thing you could do is to reuse the token array and not free it every time.

DIM macro

You have a function rsh_num_builtins(), which I would normally handle with a DIM macro:

#define DIM(array) (sizeof(array)/sizeof(array[0])) for (i = 0; i < DIM(builtins); i++) { 

Error handling

All of your functions return error values or NULL, but main never checks if line is NULL, or uses the return value of rsh_execute().

If you don't actually care about rsh_execute()'s return value, it might be more clear to just change that function to return void. However it seems like in your case you might later extend the shell to use the return value, so it's probably ok the way you have it.

Well written

Your code is written clearly with good comments. I had no trouble reading and understanding it. So good job on that.

First off rsh is a rather unfortunate name since there already was a tool with this name and it was rather widely known. I realise this is a toy and can have arbitrarily bad name, but it still should not clash with something known.

Secondly:

rsh.c: In function ‘rsh_exit’: rsh.c:42:21: warning: unused parameter ‘args’ [-Wunused-parameter] int rsh_exit(char **args) ^ rsh.c: In function ‘rsh_help’: rsh.c:67:21: warning: unused parameter ‘args’ [-Wunused-parameter] int rsh_help(char **args) ^ rsh.c: In function ‘rsh_external_execute’: rsh.c:93:11: warning: variable ‘wpid’ set but not used [-Wunused-but-set-variable] pid_t wpid; 

What's up with that?

Finally, pretty much all comments in the code were unnecessary at best and actively harmful at worst. The more comments you get, the more likely it is they will get out of sync with the code. Only comment when stuff is not obvious.

#include <stdio.h> /* printf, fprintf */ #include <stdlib.h> /* exit */ #include <string.h> /* strtok */ #include <unistd.h> /* fork, execvp */ #include <sys/wait.h> /* waitpid */ #include <sys/types.h> /* waitpid */ 

sys/ headers should got first.

Such comments quickly get out of date. In fact are already out of date - you use malloc and realloc, which require stdlib.h. I don't see them mentioned.

/* A list of builtin functions here */ char *builtins[] = { "exit", "cd", "help" }; 

Useless comment. Such lists typically end up with a NULL pointer. Consider 'static const' qualifiers.

/* * Return the number of builtin commands so we can avoid hard-coding random magic * numbers and other annoyances everywhere. */ int rsh_num_builtins(void) { return sizeof(builtins) / sizeof(char *); } 

This function, as all others (except for main) should be static.

This particular function should not exist in the first place. As was pointed out by someone else the standard way of doing this is a macro. Other approach is having aforementioned NULL at the end of the table and iterating it with a pointer. Assuming macro route, this is still bad. The idiom is to use array[0] element: sizeof(builtins) / sizeof(builtins[0]);. Even though your char * works here, there is no reason to deviate from the idiom.

/* * This builtin function changes the directory of the shell process, like the cd * in normal shells. */ int rsh_cd(char **args) { if (args[1] == NULL) { fprintf(stderr, "rsh: expected argument to cd\n"); 

You should have a dedicated function which reports errors. In particular it would know to print to stderr and would append 'rsh: ' on its own.

You ignore errno and in effect don't give any reason for the failure.

What happens if there are more arguments?

 return 0; 

The standard error would return 1, so this is reversed.

 } else if (chdir(args[1]) == -1) { fprintf(stderr, "rsh: can't cd to %s\n", args[1]); return 1; } 

There is no point doing else if and it makes the code slightly less readable. Here you return 1 on failure.

 return 0; 

.... but are back to returning 0 on success, which is the same error you return when there are no arguments.

} /* * Print out a help message about rsh */ int rsh_help(char **args) { puts("rsh: recovery shell"); puts(""); puts("rsh is a very small and simple shell that is used for system recovery purposes."); puts("It is not compliant with anything and does not allow escaping, quoting, sourcing"); puts("backslashing or anything else. It is scriptable, but only in such a way that lets"); puts("you run commands with simple options and arguments: nothing else!"); puts(""); puts("List of builtin commands:"); puts("exit cd help"); 

This should iterate over the table and print names. Otherwise it is bound to get out of sync.

 puts("All those builtins do exactly what you expect them to do, so no documentation is"); puts("needed, hopefully."); return 0; } int (*builtin_func[]) (char **) = { &rsh_exit, &rsh_cd, &rsh_help }; 

So the actual list of functions implementing builtins is detached from builtins list? This is bound to cause problems. The standard way is to obviously have a table of a struct. The struct would contain the name, the pointer and some optional stuff like expect number of arguments.

int rsh_external_execute(char **args) { pid_t pid; pid_t wpid; int status; /* fork off a new process */ pid = fork(); if (pid == 0) { /* in the child process. Note that this does not exit the parent * in an error, with the exit() call. */ if (execvp(args[0], args) == -1) { fprintf(stderr, "rsh: cannot exec\n"); exit(EXIT_FAILURE); } 

No point checking the error. If execvp returned, it failed. This should be _Exit.

 } else if (pid < 0) { 

The error check should go first for readability.

 /* error forking */ fprintf(stderr, "rsh: cannot fork\n"); return -1; } else { /* parent process */ do { wpid = waitpid(pid, &status, WUNTRACED); } while (!WIFEXITED(status) && !WIFSIGNALED(status)); 

Incorrect. waitpid can return due to an error (e.g. EINTR) in which case 'status' value is inderminate.

 } return 0; } 

[parsing code omitted, not in the mood]

/* * The main function just drops into an infinite loop of Read, Parse, Execute. * Read = use getline() to get a line from stdin, * Parse = tokenise with strtok(), * Execute = use execvp() to execute programs or execute builtin commands. */ 

Not only useless, but also bad comment. main has 0 business knowing about getline, strtok or execvp.

int main(void) { char *line; char **args; /* * This is the infinite loop of the shell. */ for (;;) { /* Check UID and print the correct prompt out */ if (geteuid() == 0) { 

UID != EUID

 printf("rsh # "); } else { printf("rsh $ "); } 

Why would you do this on each loop? Determine the right prompt beforehand and only print it when needed.

 line = rsh_readline(); args = rsh_tokenise(line); rsh_execute(args); /* Free up the unused memory */ free(line); free(args); } } 

Verdict: E-.