A basic C Shell for Linux

Question

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 <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/wait.h>

#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, "") == 0) {
                fprintf(stderr, "vsh: Expected command\n");
            }
            else if(strcmp(args[0], "cd") == 0)
            {
                directory = args[1];
                chdir(directory);
            }
            else if(strcmp(line,"programmer") == 0)
            {
                printf("The programmer of vsh is George Gibson\n");
            }
            else if(strcmp(line,"ver") == 0)
            {
                printf("The current version is 1.0\n");
            }
            else if(strcmp(line,"exit") == 0)
            {
                finish = 1;
                printf("\nLeaving vsh...\n");
                getchar();
            }
            else
            {
                pid_t pid;
                int err;

                pid = fork();

                if(pid == -1) {
                    perror("vsh");
                }
                else if(pid == 0) {
                    execvp(args[0], args);
                    perror("vsh");
                }
                else {
                    waitpid(pid, &err, 0);
                }
            }
        }
    }

    return 0;
}

I'm particularly looking for reviews relating to bugs, optimizations and general improvements.

Answer 1

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.

Answer 2

• You seem to inconsistently use conditional checks:

  • The ! operator:

    if (!tokens) {
    

  • "Yoda conditions":

    if(NULL == fgets(line, sizeof line, stdin))
    

  • Not-Yoda conditions:

    if(strcmp(line, "") == 0) {
    

  The second one can still use the ! operator (in any case with NULL), though not the third one because strcmp() returns an int. In any case, you should stick to one style for consistency and where applicable. It's one's own preference, so choose whichever you prefer.

• This doesn't look too ideal:

  if(NULL == fgets(line, sizeof line, stdin))
  {
      finish = 1;
      printf("\nLeaving vsh\n");
  }
  

  Consider making the while loop infinite by using 1 or true instead of finish. At first glance, it may not be clear to the reader that the program should end after this point since there's no return or exit(). This will help make it easier for the reader to identify all termination points of the program as opposed to scoping the entire loop flow.

• It may be necessary to terminate the program if either fork() or execvp() fails. That is usually more common, but it may still depend on how failure will affect the rest of the program.