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

A basic C Shell for Linux

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

Problem

I have written a basic C Shell. It features the builtins cd, programmer, ver and exit. The code for the shell, named vsh, is as follows:

```
#include
#include
#include
#include
#include

#define BUFSIZE 1024
#define TOK_BUFSIZE 64
#define TOK_DELIM " \t\r\n\a"

char **split_line(char *line)
{
int bufsize = TOK_BUFSIZE, position = 0;
char **tokens = malloc(bufsize sizeof(char));
char *token, **tokens_backup;

if (!tokens) {
fprintf(stderr, "vsh: allocation error\n");
exit(EXIT_FAILURE);
}

token = strtok(line, TOK_DELIM);
while (token != NULL) {
tokens[position] = token;
position++;

if (position >= bufsize) {
bufsize += TOK_BUFSIZE;
tokens_backup = tokens;
tokens = realloc(tokens, bufsize sizeof(char));
if (!tokens) {
free(tokens_backup);
fprintf(stderr, "vsh: allocation error\n");
exit(EXIT_FAILURE);
}
}

token = strtok(NULL, TOK_DELIM);
}
tokens[position] = NULL;

return tokens;
}

int main(int argc, char **argv)
{
int finish = 0;
char *user = getenv("USER");
char hostname[BUFSIZE];
char *cwd;
char line[BUFSIZE] = {0};
char **args;
char *directory;

printf("Welcome to vsh\n");

system("/bin/cat /etc/motd");

printf("\n");

gethostname(hostname, BUFSIZE);

while (!finish)
{
cwd = getcwd(cwd, BUFSIZE);

printf("[");
printf(user);
printf("@");
printf(hostname);
printf("] ");
printf(cwd);
printf(" $ ");
fflush(stdout);

if(NULL == fgets(line, sizeof line, stdin))
{
finish = 1;
printf("\nLeaving vsh\n");
}
else
{
printf("The command read was %s", line);
printf("\n");

args = split_line(line);

if(strcmp(line, "") ==

Solution

In your main() function there is a while loop (while (!finish)) and in the first few lines of that there are a lot of printf() calls. Replace all of those printf's with this one line:

printf("[%s@%s] %s $ ", user, hostname, cwd);


It is easier to read that way, and with non optimising compilers is slightly more efficient.

Additionally, your main() function contains a lot of strings with vsh in them. I recommend you replace this:

printf("vsh: allocation error\n");


with this:

printf("%s: allocation error\n", argv[0]);


That will make sure that you can more easily change the name of your shell if you want to in the future. In functions other than main() the bother of passing argv[0] may be more than it is worth, though.

Code Snippets

printf("[%s@%s] %s $ ", user, hostname, cwd);
printf("vsh: allocation error\n");
printf("%s: allocation error\n", argv[0]);

Context

StackExchange Code Review Q#99327, answer score: 3

Revisions (0)

No revisions yet.