Why does this code not get executed?

Question

I'm currently making my own shell program. I have to keep a history of the last 10 commands the user inputs. Whenever the user enters a command that ISN'T a regular command (for example history, hi, fakecommand, etc.) it is placed in history. But whenever the user inputs a real command (example ls, ps, top, cat, etc.) it is not added to history.

I think it might have something to do with the execvp command, since I believe this command creates a fork and makes the child execute the command. But I'm not sure that it should do that since I put the command in history before I even execute execvp.

//A method which increments the histIndex
int incrementIndex(int index) {
    if (index != 9)
        return index+1;
    else
        return 0;
}

//A method which adds the current command to history
void updateHistory(char *history[], char command[], int histIndex) {

    history[histIndex] = realloc(history[histIndex], MAX_LENGTH);  //Allocating space
    strcpy(history[histIndex], command);    //Put the current command in history
}

int main(int argc, char *argv[]) {
//while true, do
while (1) {

    int pid = fork();       //fork, or start the first process
    if (pid != 0) {         //if pid is not 0, then this is the parent,
        wait(NULL);
    }

    else {                  //Otherwise, we have the child

        printf("%s> ", getenv("USER"));         //print out the user's name + >
        fgets(input, MAX, stdin);               //Get the users input
        strtok(input, "\n");                    //Take the entire line and set it to input

        updateHistory(history, input, histIndex);  //Updating history
        histIndex = incrementIndex(histIndex);

        if (strcmp(input, "exit")==0) {         //If the input is exit, then exit
            exit(0);
        }

        //Else if the current command is history, then print the last 10 commands
        else if (strcmp(input, "history")==0) {
            getHistory(history, histIndex);
        }

        //Otherwise, do
        else {
            numTokens = make_tokenlist(input, tokens);

            tokens[numTokens] = NULL;
            char cmd[MAX_LENGTH];
            strcpy(cmd, tokens[0]);
            execvp(cmd, tokens);
            printf("Error: %s not found.\n", cmd);
        }
    }
}
}

Answer 1

Separate processes have their own separate memory space (unless you do something special like shared memory, etc.). Whatever updates to heap or stack structures you do in the child process (such as modifying the history), has no effect in the parent.

You are creating a child with fork(), then reading the user input in the child. The child updates its own copy of the history, which has no effect on whatever history the parent knows about. execvp() does not fork, it replaces the current process with the executed file. This replaces the entire child process, and you lose the history that you only updated in the child.

You also have children making children, which is probably not what you want, but explains why you think it is adding invalid commands to the history. (It is, but not correctly.) An illustration of the sequence of events:

    Parent
    ------
    fork()  ------>   Child
    wait()            -----
     ...              Read input
     ...              updateHistory()
     ...              exit if "exit"
     ...              print history if "history"
     ...              execvp()

                      (if execvp() succeeded, this child is consumed,
                      the executed file eventually terminates and the
                      parent stops waiting. If execvp() failed, we fall 
                      through back to the top of the while loop!)

                      fork()   --------->    Child's Child
                      wait()                 -------------
                      ...                    Read input
                      ...                    updateHistory()
                      ...                    exit if "exit"
                      ...                    print history if "history"
                      ...                    execvp()

The child's child has inherited the child's memory, so it knows about the updated history. This is why you think it is adding the failed commands to the history. It is, but it's actually much worse than that.

It seems that you should be reading input in the parent, updating history in the parent, then (given a valid command), fork off a child process to be consumed by execvp to run the command. Then let the parent wait for the child to finish. This way, the parent maintains the history. One primary purpose for forking a child in the first place is because execvp replaces the calling process. Since you want the parent to live on, you let it eat a child.

Try something like this (I'll leave it as abstract pseudocode):

    Parent
    ------
    Read input
    updateHistory()
    exit if "exit"
    print history if "history"
    if invalid, go back to [Read input]
    if valid:
        fork()  ------>   Child
        wait()            -----
        ...               execvp()
        ...     <-------  if successful, executable hopefully terminates
        ...     <-------  if failed, print error and exit
                          (either way, child ends)

    Parent goes back to [Read input]

Another thing worth mentioning, whenever you fork(), you should check for three possible return values: -1 (error in fork()), >0 (in parent process), and 0 (in child process).