HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Recovery Shell (rsh)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
recoveryrshshell

Problem

I wrote a shell to test my understanding of processes etc. It is not supposed to comply with POSIX or anything, just to allow people to run simple commands with simple arguments.

```
#include / printf, fprintf /
#include / exit /
#include / strtok /
#include / fork, execvp /
#include / waitpid /
#include / waitpid /
/*
* This program is a simple, first-level shell for testing and development purposes.
* It is called rsh: the Recovery SHell because it can be used for recovery as it has
* no dependencies apart from the C standard library and is very small and simple.
* This shell does not follow POSIX or anything else: it is purely for executing
* commands with simple arguments (ie no quote escape, backslash escape, etc)
*/

/*
* Function Return Values:
* All functions in this program return 0 on success and -1 on failure, unless they
* return useful information, in which the return values will be documented in a
* comment at the top of the function. With the exception of builtins. Builtin
* commands return 0 for success and 1 for not successful, like the real commands.
*/

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

/*
* 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 builtin function exits the shell.
*/
int rsh_exit(char **args)
{
exit(EXIT_SUCCESS);
}

/*
* 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");
return 0;
} else if (chdir(args[1]) == -1) {
fprintf(stderr, "rsh: can't cd to %s\n", args[1]);
return 1;
}

return 0;
}

/*
* Print out a help message about rsh
*/
int rsh_help(char **args)
{
puts("rs

Solution

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:

  • Do one quick pass over the input to count the number of tokens.



  • 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.

Code Snippets

#define DIM(array) (sizeof(array)/sizeof(array[0]))

for (i = 0; i < DIM(builtins); i++) {

Context

StackExchange Code Review Q#90787, answer score: 7

Revisions (0)

No revisions yet.