Implementing input/output redirection in a Linux shell using C

Question

I am trying to create a basic shell for Linux using C. I have gotten it to work until I try to do output redirection and it just destroys everything. When I run this code, it goes straight to the default case of the fork(). I have no idea why. If I get rid of the for loop in the child process it works, but even with the for loop I don't understand why the child process is never even entered. If I put a print statement at the top of the child process it doesn't get printed.

When I run this in command line, I get the prompt and type something like "ls", which worked before I added the for loop, but now I just get "% am i here" and if I press enter it just keeps giving me that same line. My goal is to be able to type "ls > output" and have it work. I think the input redirection works, but honestly I haven't even played with it much because I am so utterly confused as to what is going on with the output redirection. Any help would be greatly appreciated, I've spent 4 hours on the same like 15 lines trying to get this to work.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/wait.h>
#define HISTORY_COUNT 20
char *prompt = "% ";

int
main()
{
int pid;
//int child_pid;
char line[81];
char *token;
char *separator = " \t\n";
char **args;
char **args2;
char **args3;
char cmd[100];
char *hp;
char *cp;
char *ifile;
char *ofile;
int check;
int pfds[2];
int i;
int j;
int current = 0;
int p = 0;
//int check;
char *hist[HISTORY_COUNT];
//char history[90];
//typedef void (*sighandler_t) (int);

args = malloc(80 * sizeof(char *));
args2 = malloc(80 * sizeof(char *));

signal(SIGINT, SIG_IGN);

while (1) {
    fprintf(stderr, "%s", prompt);
    fflush(stderr);

    if (fgets(line, 80, stdin) == NULL)
        break;


    // split up the line

    i = 0;
    while (1) {
        token = strtok((i == 0) ? line : NULL, separator);
        if (token == NULL)
            break;
        args[i++] = token;

             /* build command array */

    }
    args[i] = NULL;

    if (i == 0){
        continue;
    }

    // assume no redirections
    ofile = NULL;
    ifile = NULL;

    // split off the redirections
    j = 0;
    i = 0;
    while (1) {        //stackoverflow.com/questions/35569673
        cp = args[i++];
        if (cp == NULL)
            break;

        switch (*cp) {
        case '<':
            if (cp[1] == 0)
                cp = args[i++];
            else
                ++cp;
            ifile = cp;
            break;

        case '>':
            if (cp[1] == 0)
                cp = args[i++];
            else
                ++cp;
            ofile = cp;
            break;
    case '|':

        if(cp[1] ==0){
           cp = args[i++];
           if(pipe(pfds) == -1){
               perror("Broken Pipe");
               exit(1);
           }
       p = 1;
    }
    else{ 
       ++cp;

    }
       break;

        default:
            args2[j++] = cp;
        args3[cp++] = cp
            break;
        }
    }
    args2[j] = NULL;
    if (j == 0)
        continue;

    switch (pid = fork()) {
        case 0:
            // open stdin
            if (ifile != NULL) {
                int fd = open(ifile, O_RDONLY);

                if (dup2(fd, STDIN_FILENO) == -1) {
                    fprintf(stderr, "dup2 failed");
                }

                close(fd);
            }


            // open stdout
            if (ofile != NULL) {
                // args[1] = NULL;
                int fd2;


                if ((fd2 = open(ofile, O_WRONLY | O_CREAT, 0644)) < 0) {
                    perror("couldn't open output file.");
                    exit(0);
                }

                // args+=2;
                printf("okay");
                dup2(fd2, STDOUT_FILENO);
                close(fd2);
            }


        if(p == 1){        //from stackoverflow.com/questions/2784500
        close(1);
        dup(pfds[1]);
        close(pfds[0]);
        execvp(args2[0], args2);
        break;
        }


        if(strcmp(args2[0], "cd") == 0){            //cd command
            if(args2[1] == NULL){
                fprintf(stderr, "Expected argument");
            }
            else{

            check = chdir(args2[1]);

        if(check != 0){
            fprintf(stderr,"%s",prompt);

            }
           }
        break;
        }

        execvp(args2[0], args2);        /* child */
        signal(SIGINT, SIG_DFL);
        fprintf(stderr, "ERROR %s no such program\n", line);
        exit(1);
        break;

    case -1:
        /* unlikely but possible if hit a limit */
        fprintf(stderr, "ERROR can't create child process!\n");
        break;

    default:
        //printf("am I here");
    if(p==1){
        close(0);
        dup(pfds[0]);
        close(pfds[1]);
        //execvp();
    }
        wait(NULL);
        //waitpid(pid, 0, 0);
    }
}

exit(0);

}

Answer 1

I added a separate argument pass to capture and remember the I/O redirections and remove them from the arg list passed to the child.

Here's the corrected code [please pardon the gratuitous style cleanup]:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <signal.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/wait.h>

char *prompt = "% ";

int
main()
{
    int pid;
    //int child_pid;
    char line[81];
    char *token;
    char *separator = " \t\n";
    char **args;
    char **args2;
    char *cp;
    char *ifile;
    char *ofile;
    int i;
    int j;
    int err;
    //int check;
    //char history[90];
    //typedef void (*sighandler_t) (int);

    args = malloc(80 * sizeof(char *));
    args2 = malloc(80 * sizeof(char *));

    //signal(SIGINT, SIG_IGN);

    while (1) {
        fprintf(stderr, "%s", prompt);
        fflush(stderr);

        if (fgets(line, 80, stdin) == NULL)
            break;

        // split up the line
        i = 0;
        while (1) {
            token = strtok((i == 0) ? line : NULL, separator);
            if (token == NULL)
                break;
            args[i++] = token;              /* build command array */
        }
        args[i] = NULL;
        if (i == 0)
            continue;

        // assume no redirections
        ofile = NULL;
        ifile = NULL;

        // split off the redirections
        j = 0;
        i = 0;
        err = 0;
        while (1) {
            cp = args[i++];
            if (cp == NULL)
                break;

            switch (*cp) {
            case '<':
                if (cp[1] == 0)
                    cp = args[i++];
                else
                    ++cp;
                ifile = cp;
                if (cp == NULL)
                    err = 1;
                else
                    if (cp[0] == 0)
                        err = 1;
                break;

            case '>':
                if (cp[1] == 0)
                    cp = args[i++];
                else
                    ++cp;
                ofile = cp;
                if (cp == NULL)
                    err = 1;
                else
                    if (cp[0] == 0)
                        err = 1;
                break;

            default:
                args2[j++] = cp;
                break;
            }
        }
        args2[j] = NULL;

        // we got something like "cat <"
        if (err)
            continue;

        // no child arguments
        if (j == 0)
            continue;

        switch (pid = fork()) {
        case 0:
            // open stdin
            if (ifile != NULL) {
                int fd = open(ifile, O_RDONLY);

                if (dup2(fd, STDIN_FILENO) == -1) {
                    fprintf(stderr, "dup2 failed");
                }

                close(fd);
            }

            // trying to get this to work
            // NOTE: now it works :-)
            // open stdout
            if (ofile != NULL) {
                // args[1] = NULL;
                int fd2;

                //printf("PLEASE WORK");
                if ((fd2 = open(ofile, O_WRONLY | O_CREAT, 0644)) < 0) {
                    perror("couldn't open output file.");
                    exit(0);
                }

                // args+=2;
                printf("okay");
                dup2(fd2, STDOUT_FILENO);
                close(fd2);
            }

            execvp(args2[0], args2);        /* child */
            signal(SIGINT, SIG_DFL);
            fprintf(stderr, "ERROR %s no such program\n", line);
            exit(1);
            break;

        case -1:
            /* unlikely but possible if hit a limit */
            fprintf(stderr, "ERROR can't create child process!\n");
            break;

        default:
            //printf("am I here");
            wait(NULL);
            //waitpid(pid, 0, 0);
        }
    }

    exit(0);
}

---

UPDATE:

If you're still around do you think you could help me with creating a pipe?

Sure. It's too big to post here. See: http://pastebin.com/Ny1w6pUh

---

Wow did you create all 3300 lines?

Yes.

I borrowed xstr from another SO answer of mine [with bugfix and enhancement]. The dlk was new, but I do many of those, so was easy. Most of it was new code.

But ... It was composed of fragments/concepts I've done before: tgb, FWD, BTV, sysmagic. Notice all struct members for struct foo are prefixed with foo_ [standard for me]. The macro "trickery" with DLHDEF and DLKDEF to simulate inheritance/templates is also something I do [when necessary].

Many function vars reuse my style: idx for index var [I would never use i/j, but rather xidx/yidx], cp for char pointer, cnt for count, len for byte length, etc. Thus, I don't have to "think" about small stuff [tactics] and can focus on strategy.

The above idx et. al. is a "signature style" for me. It's not necessarily better [or worse] than others. It comes from the fact that I started using C when the linker/loader could only handle 8 character symbols, so brevity was key. But, I got used to using the shorter names. Consider which is clearer/better:

for (fooidx = 0;  fooidx <= 10;  ++fooidx)

Or:

for (indexForFooArray = 0;  indexForFooArray <= 10;  ++indexForFooArray)

I use the do { ... } while (0) to avoid if/else ladders a lot. This is called a "once through" loop. This is considered "controversial", but, in my experience it keeps the code cleaner. Personally, I've never found a [more standard] use of a do/while loop that can't be done more easily/better with a while or for loop--YMMV. In fact, a number of languages don't even have do/while at all.

Also, I use lower case unless it's a #define [or enum] which is always upper. That is, I use "snake case" (e.g. fooidx) and not "camel hump case" (e.g. indexForFooArray).

The .proto file containing function prototypes is auto-generated. This is a huge time saver. Side note: Be sure you have at least v2 from the external link as there was a bug in the Makefile. A make clean would erase the .proto. v2 won't do that

I've developed my own style over the years. Turns out that the linux kernel style was "borrowed from mine". Not actually :-) Mine came first. But ... They, in parallel, came up with something that is a 99% match to mine: /usr/src/kernels/whatever_version/Documentation/CodingStyle.

Consistency to a given style [one's own] is key. For a given function, you don't have to worry about what you'll name the variables, what indent or blank lines you'll use.

This helps a reader/new developer. They can read a few functions, see the style in play, and then go faster because all functions have similar style.

All this allows you to "go faster" and still get high quality code on the first try. I'm also quite experienced.

Also, my code comments focus on "intent". That is, what do you want the code to do in real world terms. They should answer the "what/why" and the code is the "how".