C homework assignment, pipes and processes

Question

I've asked many, many C questions here on stackoverflow over the past few days. I feel like I'm at the final one. My assignment works ALMOST perfectly. Here is the premise:

Write a program to query the user for two input strings. Each input string should be a unix command, with arguments allowed. For example, input 1 could be ls -l and input 2 could be more. The program will then create a pipe, and two child processes. The first child process will run the command specified in the first input. It will output to the pipe instead of standard output. The second child process will run the command specified in the second input. It will take its input from the pipe rather than standard input. The parent process will wait on its two children to complete, then the whole thing will repeat. Execution will stop when the '@' symbol is entered as the first command. Here is the code I have:

#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

int main(){

    /* Program Termination Symbol */
    const char terminate = '@';

    /* String delimiter */
    const char delimiter = ' ';

    /* Pipe file ID's */
    int fileID[2];

    /* Parent ID's */
    int pid1, pid2;

    /* String token */
    char * token, * token2;

    /* User input */
    char * user_input, line[100];

    user_input = (char *) malloc(100);

    /* Unix Commands */
    char * command1[10], *command2[10];

    for (int i=0; i<10; i++)
    {
    command1[i] = (char *)malloc(100*sizeof(char));
    command2[i] = (char *)malloc(100*sizeof(char));
    }

    /* Begin main program logic */

    printf("Please enter the first command: \n");

    user_input = gets(line);

    while (user_input[0] != terminate)
    {
    token = (char *) malloc(100*sizeof(char));

    for (int i=0; i<10; i++)
        {
        if (i == 0)
        {
        token = strtok(user_input, &delimiter); 
        } else {
        token = strtok(NULL, &delimiter);
        }

        if (token != NULL)
        {
        strcpy(command1[i], token);
        } else {
        command1[i] = 0;
        }
        }

    printf("Please enter the second command: \n");  
    user_input = gets(line);

    token2 = (char *) malloc(100*sizeof(char));

    for (int i=0; i<10; i++)
    {
        if (i == 0)
        {
        token2 = strtok(user_input, &delimiter);
        } else {
        token2 = strtok(NULL, &delimiter);
        }

        if (token2 != NULL)
        {
        strcpy(command2[i], token2);
        } else {
        command2[i] = 0;
        }
    }   


    /* Pipe and execute user commands */

    /* Create pipe */
    pipe(fileID);

    /* Create child processes */

    pid1 = fork();

    if (pid1 != 0)
    {
        pid2 = fork();
    }

    /* First child process */
    if (pid1 == 0)
    {
        dup2(fileID[1], 1);
        execvp(command1[0], command1);
    }

    /* Second child process */
    if (pid2 == 0)
    {
        dup2(fileID[0], 0);
        execvp(command2[0], command2);
    }   

    /* Wait for children to terminate */
    wait(&pid1);
    wait(&pid2);

    /* Repeat */
        printf("Please enter the first command: \n");
    user_input = gets(line);
    }

    return 0;
}

The problem I'm running into is with my waits. If I have both, which would make sense to me (one wait per child) then the program freezes after executing the first pipe. If I remove the second wait, then the program will begin its loop again, but will not accept keyboard input other than enter, and will produce a segfault. So, with both waits, input and output is...

Please enter the first command:
ls
Please enter the second command:
more
Pipe
Pipe.c
Pipe.c~

...and then it locks up. If I remove the second wait, input/output is...

Please enter the first command:
ls
Please enter the second command:
more
Pipe
Pipe.c
Pipe.c~
Please enter the first command:
(I hit enter, nothing else will work)
Segmentation fault

Anyone have any suggestions? It's clearly related to waiting on the two processes, but I'm at a loss as to how to handle it.

Thanks.

EDIT: This program is now 100% functional - thank you so much for your help, everyone! Stack overflow has been one of the best resources on the internet. Thank you all so much for taking the time to look over my code and give me your suggestions.

Answer 1

A couple of things. Not sure that they are the cause of your problems, but still things to consider before submitting your homework.

1. I don't think you are using wait correctly. According to http://linux.die.net/man/2/wait it doesn't take pid pointer as an argument.

2. Each time around the loop you call malloc for token and token2, but I don't see a corresponding release of the memory.

3. You've writen a single monolithic function. Good coding practice would suggest breaking it out into a collection of subroutines

4. Finally, and its possibly related to point 3, the following two lines of code appear twice in your code. Again it's not an error, but unnecessary duplication, and inelegant.

   printf("Please enter the first command: \n"); user_input = gets(line);

Answer 2

First of all, you're calling wait from the child processes as well [edit: no, you're not, since each child calls execvp].

Also, wait doesn't take a pointer to the child's pid, but to a variable where the process's status will be written to (which means you're throwing away your child's pid).

Finally, try using waitpid with the "WNOHANG" option. It won't hang, you can put both on a loop while you do other stuff, and you can check to see if the child processes have exited by inspecting the status variables. man waitpid.

Answer 3

I agree with everything torak said, but to address your problem, you need to close your pipes. I think you are "hanging" because the pipe is still open.

So in the parent, right before the "waits", I would close the pipes.

close(fileID[0]);
close(fileID[1]);
wait(&pid_status);
wait(&pid_status);

Then, right before each execvp, I would close the ends of the pipe the child will not be using:

close(fileID[0]);
dup2(fileID[1], 1);
execvp(command1[0], command1);


close(fileID[1]);
dup2(fileID[0], 0);
execvp(command2[0], command2);

That should resolve your hanging. In addition to the suggestions made by torak, I would also recommend fgets instead of gets to prevent a buffer overflow.