19
\$\begingroup\$

I wrote this shell in C about a semester ago for a university assignment on operating systems. Even though I got a score of 10/10 in this assignment, I doubt it deserved it -

  • The pipeline implementation could be written better.
  • There were some cases (1%) where I got the pipeline to stuck while bash did not.
  • Maybe some functions could be broken down into smaller/simpler ones.
  • The debug messages (although they were optional) could be written in a log file.

We were not allowed to split the source code into separate files

Here is the source code for it.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#include <string.h>

struct subcommand_t {
    char **argument; // Array of arguments
    size_t size;     // Number of arguments
};

struct command_t {
    struct subcommand_t *subcommand; // Array of subcommands.
    size_t size;                     // Number of subcommands.
};

struct line_t {
    struct command_t *command; // Array of commands.
    size_t size;               // Number of commands.
};

void prompt(char *prompt) {
    #ifdef DEBUG
    fprintf(stderr, "[PROMPT] Displaying prompt\n");
    #endif
    fprintf(stdout, "%s ", prompt);
}

char *reader(size_t size){
    char *buffer;
    #ifdef DEBUG
    fprintf(stderr, "[READER] Allocating memory for buffer [size: %zu]\n", size);
    #endif
    buffer = malloc(sizeof(char) * size);
    if (buffer == NULL) exit(EXIT_FAILURE);
    #ifdef DEBUG
    fprintf(stderr, "[READER] Reading to buffer\n");
    #endif
    int character;
    size_t length = 0;
    while (EOF != (character = fgetc(stdin)) && character != '\n') {
        buffer[length++] = (char) character;
        if (length == size) {
            #ifdef DEBUG
            fprintf(stderr, "[READER] Reallocating memory for buffer [size: %zu]\n", size);
            #endif
            buffer = realloc(buffer, sizeof(char) * (size += 32));
            if (buffer == NULL) exit(EXIT_FAILURE);
        }
    }
    #ifdef DEBUG
    fprintf(stderr, "[READER] Setting the NULL terminator for buffer\n");
    #endif
    buffer[length++] = '\0';
    return realloc(buffer, sizeof(char) * length);
}

void executor(struct line_t line) {
    for (size_t i = 0; i < line.size; ++i) {
        #ifdef DEBUG
        fprintf(stderr, "[EXECUTOR] Executing line.command[%zu]\n", i);
        #endif
        int previous;
        for (size_t j = 0; j < line.command[i].size; ++j) {
            #ifdef DEBUG
            fprintf(stderr, "[EXECUTOR] Executing line.command[%zu].subcommand[%zu]\n", i, j);
            #endif
            int p[2];
            pipe(p);
            if (line.command[i].subcommand[j].size != 0) {
                if (strcmp(line.command[i].subcommand[j].argument[0], "exit") == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[EXECUTOR] Exiting shell\n");
                    #endif
                    exit(EXIT_SUCCESS);
                } else if (strcmp(line.command[i].subcommand[j].argument[0], "cd") == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[EXECUTOR] Changing directory to %s\n", line.command[i].subcommand[j].argument[1]);
                    #endif
                    chdir(line.command[i].subcommand[j].argument[1]);
                } else {
                    switch(fork()) {
                        case -1:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Failed to fork a child process\n");
                            #endif
                            exit(EXIT_FAILURE);
                        case  0:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Running on a child process [pid: %d]\n", getpid());
                            #endif
                            if (j == 0) {
                                dup2(0, STDIN_FILENO);
                            } else {
                                dup2(previous, STDIN_FILENO);
                            }
                            close(p[0]);
                            if (j+1 < line.command[i].size) {
                                dup2(p[1],STDOUT_FILENO);
                            }
                            close(p[0]);
                            execvp(line.command[i].subcommand[j].argument[0], line.command[i].subcommand[j].argument);
                            exit(EXIT_FAILURE);
                        default:
                            #ifdef DEBUG
                            fprintf(stderr, "[EXECUTOR] Running on the parent process [%d]\n", getpid());
                            #endif
                            previous=p[0];
                            close(p[1]);
                    }
                }
            }
        }
        #ifdef DEBUG
        fprintf(stderr, "[EXECUTOR] Waiting for any terminated child processes\n");
        #endif
        while(wait(NULL) > 0) {
            #ifdef DEBUG
            fprintf(stderr, "[EXECUTOR] Found a terminated child process\n");
            #endif
        }
    }
}

struct line_t parser(char *buffer, char *del1, char *del2, char *del3) {
    size_t i, j, k;
    char *str1, *str2, *str3;
    char *token1, *token2, *token3;
    char *saveptr1, *saveptr2, *saveptr3;

    struct line_t line;
    line.size=0;
    line.command=NULL;
    for (i = 0, str1 = buffer ;; i++, str1 = NULL) {
        token1 = strtok_r(str1, del1, &saveptr1);
        if (token1 == NULL) break;
        line.size++;
        if (i == 0) {
            #ifdef DEBUG
            fprintf(stderr, "[PARSER] Allocating memory for line.command [size: %zu]\n", line.size);
            #endif
            line.command = malloc(sizeof(struct command_t));
        } else {
            #ifdef DEBUG
            fprintf(stderr, "[PARSER] Reallocating memory for line.command [size: %zu]\n", line.size);
            #endif
            line.command = realloc(line.command, line.size * sizeof(struct command_t));
        }
        line.command[i].size=0;
        line.command[i].subcommand=NULL;
        for (j = 0, str2 = token1 ;; j++, str2 = NULL) {
            token2 = strtok_r(str2, del2, &saveptr2);
            if (token2 == NULL) break;
            line.command[i].size++;
            if (j == 0) {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Allocating memory for line.command[%zu].subcommand [size: %zu]\n", i, line.command[i].size);
                #endif
                line.command[i].subcommand = malloc(sizeof(struct subcommand_t));
            } else {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Reallocating memory for line.command[%zu].subcommand [size: %zu]\n", i, line.command[i].size);
                #endif
                line.command[i].subcommand = realloc(line.command[i].subcommand, line.command[i].size * sizeof(struct subcommand_t));
            }
            line.command[i].subcommand[j].size=0;
            line.command[i].subcommand[j].argument=NULL;
            for (k = 0, str3 = token2 ;; k++, str3 = NULL) {
                token3 = strtok_r(str3, del3, &saveptr3);
                if (token3 == NULL) break;
                line.command[i].subcommand[j].size++;
                if (k == 0) {
                    #ifdef DEBUG
                    fprintf(stderr, "[PARSER] Allocating memory for line.command[%zu].subcommand[%zu].argument [size: %zu]\n", i, j, line.command[i].subcommand[j].size);
                    #endif
                    line.command[i].subcommand[j].argument = malloc(sizeof(char *));
                } else {
                    #ifdef DEBUG
                    fprintf(stderr, "[PARSER] Reallocating memory for line.command[%zu].subcommand[%zu].argument [size: %zu]\n", i, j, line.command[i].subcommand[j].size);
                    #endif
                    line.command[i].subcommand[j].argument = realloc(line.command[i].subcommand[j].argument, (line.command[i].subcommand[j].size + 1) * sizeof(char *));
                }
                line.command[i].subcommand[j].argument[k] = malloc((strlen(token3)+1) * sizeof(char));
                memset(line.command[i].subcommand[j].argument[k], 0, strlen(token3)+1);
                strcpy(line.command[i].subcommand[j].argument[k], token3);
            }
            if (line.command[i].subcommand[j].size != 0) {
                #ifdef DEBUG
                fprintf(stderr, "[PARSER] Setting the NULL terminator for line.command[%zu].subcommand[%zu]\n", i, j);
                #endif
                line.command[i].subcommand[j].argument[line.command[i].subcommand[j].size] = NULL;
            }
        }
    }
    return line;
}

int main() {
    while (1) {
        prompt("$");
        char *buffer = reader(1024);
        struct line_t line = parser(buffer, ";", "|", " \t");
        executor(line);
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ It's worth noting that, in a university assignment, your goal isn't "be perfect", it's "do enough to prove that you're learning the course content." Similarly, in a job, your goal still isn't perfection -- unless you're in a toxic work environment, in which case, find a new job before you burn out -- but satisfying whoever the users of the software will be. Your code will never be perfect, be it because of lack of knowledge, time/budget constraints, other priorities, new things that didn't exist... And so on. Your goal is to write the best code that accomplishes the goals. You did that. \$\endgroup\$ – Nic Hartley Apr 13 '18 at 17:05
  • \$\begingroup\$ @NicHartley Hmmm... I see what you mean. \$\endgroup\$ – xorz57 Apr 13 '18 at 17:10
  • \$\begingroup\$ Here is a simplified version of the same shell github.com/xorz57/shell It is memory-leak free \$\endgroup\$ – xorz57 May 3 at 23:18
21
\$\begingroup\$

You wrote a nice and simple shell. It works for very simple commands but fails on more complex ones (for details, see below). The code can be cleaned up in several places.


To make your code nicely readable by others, let an automatic formatter take care of the indentation and spacing. If you have GNU Indent available, use the following command line:

indent --k-and-r-style --no-tabs --line-length 200 --case-indentation 4 --braces-on-func-def-line shell.c

Instead of writing this every time:

#ifdef DEBUG
fprintf(stderr, "[READER] Allocating memory for buffer [size: %zu]\n", size);
#endif

you should define a macro DEBUG_PRINTF:

#ifdef DEBUG
#define DEBUG_PRINTF(...) fprintf(stderr, __VA_ARGS__)
#else
#define DEBUG_PRINTF(...) (void)0
#endif

Then you can just write:

DEBUG_PRINTF("[READER] Allocating memory for buffer [size: %zu]\n", size);

if (buffer == NULL) exit(EXIT_FAILURE);

Before exiting with an error code, you should print an error message. By convention, an empty output means success.


Enable all compiler warnings and fix them properly. For GCC, these are -Wall -Wextra -Werror -O2.

  • Instead of void prompt, write static void prompt. This makes the function local to the current file and avoids conflicts just in case another file defines another function of the same name. (Also for the other functions.)

  • Instead of int main(), write int main(void) to fix the "missing prototype" warning.

  • Declare all read-only strings as const char * instead of char *. This affects the parameters to prompt and to parser.


Rename your functions. Function names are usually verbs. Your current names are executor, parser, etc. These are readable and understandable but still should be execute and parse.

For reader, this is more difficult since there is a system-provided function called read. Therefore it should be renamed to read_line.


Declare variables directly where you need them. Instead of:

char *buffer;
buffer = malloc(sizeof(char) * size);

just write:

char *buffer = malloc(sizeof(char) * size);

Since sizeof(char) is defined to always be 1, leave it out:

char *buffer = malloc(size);

In the executor function, instead of repeating line.command[i].subcommand[j] everywhere, define a new variable:

for (size_t j = 0; j < line.command[i].size; ++j) {
    subcommand_t subcommand = line.command[i].subcommand[j];

    if (subcommand.size != 0) {
        // ...

The parser function currently parses the following command unexpectedly:

echo ";"

It outputs " but doesn't give any hint on why the semicolon and the second quotation mark were not echoed.


memset(line.command[i].subcommand[j].argument[k], 0, strlen(token3)+1);
strcpy(line.command[i].subcommand[j].argument[k], token3);

The memset is redundant and should be removed.


Any memory that you allocate with malloc or realloc must be freed after use with free.


When I press Ctrl+D in your shell, I get stuck in an endless loop. All other shells exit at that point (or instruct me that I must type exit instead of just Ctrl+D, which is really annoying).

To handle this situation, add error handling in the reader function and return from main if its return value is NULL.

\$\endgroup\$
  • \$\begingroup\$ First of all, I would like to thank you for your professional review. I will follow your recommendations when I have the time and post a follow-up question linked to this. As for the failing echo command, it's a shell built-in command that I have not implemented because it was not required for the assignment I was given. This could be implemented in the execute() function. \$\endgroup\$ – xorz57 Apr 13 '18 at 15:14
  • \$\begingroup\$ I also forgot to mention that the output was checked against an automated grading system which is why I did not include warning messages. \$\endgroup\$ – xorz57 Apr 14 '18 at 23:59
  • 1
    \$\begingroup\$ @xorz57 Regarding the echo command: nothing I said was specific to the echo command, or whether the command is built-in or external. It was just about parsing of the commands. \$\endgroup\$ – Roland Illig Apr 15 '18 at 6:34
  • \$\begingroup\$ Regarding the warnings: would your program pass the tests if you printed the warnings on stderr? The tests should allow this since it is common practice. \$\endgroup\$ – Roland Illig Apr 15 '18 at 6:37
  • 1
    \$\begingroup\$ Our professor didn't allow us to print any extra mesaages during the unit tests. Thats why i put the DEBUG flag. \$\endgroup\$ – xorz57 Apr 15 '18 at 9:46
6
\$\begingroup\$

Use of _t

You may want to reconsider use of _t endings in your type names. In short, most of the standard type names use this, and the general practice is to not do so with user-defined types.

To quote POSIX:

To allow implementors to provide their own types, all conforming applications are required to avoid symbols ending in "_t", which permits the implementor to provide additional types.

As a basic example:

typedef struct Subcommand {
    char **argument; // Array of arguments
    size_t size;     // Number of arguments
} Subcommand;

There's also more information in this Stack Overflow question that may be helpful.

\$\endgroup\$
  • \$\begingroup\$ I don't necessarily agree with the POSIX recommendation myself, though it does point out a very important limitation of C in general, i.e. the single namespace for typedef. \$\endgroup\$ – Greg A. Woods Apr 13 '18 at 17:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.