21

Round 2

After reading some of the answers, my revised code is:

int pid = fork();

if (pid == -1) {
    perror("fork");
} else if (pid == 0) {   

    if (in) { //if '<' char was found in string inputted by user
        int fd0 = open(input, O_RDONLY, 0);
        dup2(fd0, STDIN_FILENO);
        close(fd0);
        in = 0;
    }

    if (out) { //if '>' was found in string inputted by user
        int fd1 = creat(output, 0644);
        dup2(fd1, STDOUT_FILENO);
        close(fd1);
        out = 0;
    }   

    execvp(res[0], res);
    perror("execvp");
    _exit(1);
} else {
    waitpid(pid, 0, 0);
    free(res);
}

It works, but seems the standard output isn't being reconnected or something to that effect. Here is execution:

SHELL$ cat > file
hello, world
this is a test
SHELL$ cat < file //no output
SHELL$ ls //no output

'<' and '>' both work, but after they are executed there is no output.


Round 1

I have been working on a relatively simple shell in C for a while now, but I am having trouble implementing input (<) and output (>) redirection. Help me find the issues in the following code:

int fd;
int pid = fork();
int current_out;

if (in) { //if '<' char was found in string inputted by user
    fd = open(input, O_RDONLY, 0);
    dup2(fd, STDIN_FILENO);
    in = 0;
    current_out = dup(0);
}

if (out) { //if '>' was found in string inputted by user
    fd = creat(output, 0644);
    dup2(fd, STDOUT_FILENO);
    out = 0;
    current_out = dup(1);
}

if (pid == -1) {
    perror("fork");
} else if (pid == 0) {       
    execvp(res[0], res);
    perror("execvp");
    _exit(1);
} else {
    waitpid(pid, 0, 0);
    dup2(current_out, 1);
    free(res);
}

I may have some unnecessary material in there because I have been trying different things to get it to work. I am not sure what is going wrong.

3
  • 1
    current_out feels very out of place; dup(0), for example, duplicates the standard input descriptor, not standard out. Are you confident you have parsed in and input correctly? Add some debugging printf() calls around the open and dup2 -- or use strace(1) to watch the system calls you are making. Commented Jul 17, 2012 at 2:59
  • VERY confident about in and input. Not confident about anything dealing with handling input redirection; it is very new and strange to me. Commented Jul 17, 2012 at 3:00
  • 2
    You should not use "0" some times and STDIN_FILENO other times. Pick one or the other. I'm not sure why you are saving dup(0) as current_out. Surely current_in is better (though honestly keeping another ref around is not helping anything). Running strace -o/tmp/tr -f myshell 'cat < /etc/passwd' and showing us the system calls between the open and the fork will make things much clearer, and then showing us what the child (cat) does would be good as well. Commented Jul 17, 2012 at 3:01

3 Answers 3

23

You have way too many file descriptors open after your redirection. Let's dissect the two paragraphs:

if (in) { //if '<' char was found in string inputted by user
    fd = open(input, O_RDONLY, 0);
    dup2(fd, STDIN_FILENO);
    in = 0;
    current_in = dup(0);  // Fix for symmetry with second paragraph
}

if (out) { //if '>' was found in string inputted by user
    fd = creat(output, 0644);
    dup2(fd, STDOUT_FILENO);
    out = 0;
    current_out = dup(1);
}

I'm going to be charitable and ignore the fact that you are ignoring errors. However, you will need to error check your system calls.

In the first paragraph, you open a file and capture the file descriptor (it might well be 3) in the variable fd. You then duplicate the file descriptor over standard input (STDIN_FILENO). Note, though, that file descriptor 3 is still open. Then you do a dup(0) (which, for consistency, should be STDIN_FILENO), getting another file descriptor, perhaps 4. So you have file descriptors 0, 3 and 4 pointing at the same file (and, indeed, the same open file description — noting that an open file description is different from an open file descriptor). If your intention with current_in was to preserve the (parent) shell's standard input, you have to do that dup() before you do the dup2() that overwrites the output. However, you would be better off not altering the parent shell's file descriptors; it is less overhead than re-duplicating the file descriptors.

Then you more or less repeat the process in the second paragraph, first overwriting the only record of file descriptor 3 being open with the fd = creat(...) call but obtaining a new descriptor, perhaps 5, then duplicating that over standard output. You then do a dup(1), yielding another file descriptor, perhaps 6.

So, you have stdin and stdout of the main shell redirected to the files (and no way of reinstating those to the original values). Your first problem, therefore, is that you are doing the redirection before you fork(); you should be doing it after the fork() — though when you get to piping between processes, you will need to create pipes before forking.

Your second problem is that you need to close a plethora of file descriptors, one of which you no longer have a reference for.

So, you might need:

if ((pid = fork()) < 0)
    ...error...
else if (pid == 0)
{
    /* Be childish */
    if (in)
    {
        int fd0 = open(input, O_RDONLY);
        dup2(fd0, STDIN_FILENO);
        close(fd0);
    }

    if (out)
    {
        int fd1 = creat(output , 0644) ;
        dup2(fd1, STDOUT_FILENO);
        close(fd1);
    }
    ...now the child has stdin coming from the input file, 
    ...stdout going to the output file, and no extra files open.
    ...it is safe to execute the command to be executed.
    execve(cmd[0], cmd, env);   // Or your preferred alternative
    fprintf(stderr, "Failed to exec %s\n", cmd[0]);
    exit(1);
}
else
{
    /* Be parental */
    ...wait for child to die, etc...
}

Before you do any of this, you should ensure that you've already flushed the shell's standard I/O channels, probably by using fflush(0), so that if the forked child writes to standard error because of a problem, there is no extraneous duplicated output.

Also note that the various open() calls should be error-checked.

Sign up to request clarification or add additional context in comments.

6 Comments

You can do some simple validation of whether the parent shell's standard output is messed up by putting appropriate printf() and fflush() calls in the parent-only code (e.g. after you've captured the exit status of the child, you could print out its pid and its exit status). Your original code had dup2(current_out, FILENO_STDOUT); did you get rid of that? (The answer is probably yes, but if you didn't, it could account for your problem.)
Yes, I did get rid if it. The parent is printing to the console and the child seems to be the problem.
Ok, looks like it was a simple fix. "in" and "out" were being set to 0 in the child process, but that parent still stored the previous values from the last iteration of my infinite while loop controlling the shell.
I like the be childish comment. I’m gonna use that with my students.
@CharlieMartin — you're welcome to do so. I like the nomenclature. I have a trio of programs from yesteryear where I have functions static void be_childish(char **argv, Pipe to_sqlexec, Pipe from_sqlexec) { … } and static void be_parental(Pipe to_sqlexec, Pipe from_sqlexec) { … } which are given appropriate data structures (typedef int Pipe[2];) and argument lists. It's reasonably clear what those functions are doing — at least if you know what the sqlexec means (it was for an ancient iteration of an SQL DBMS engine).
|
7

You have way too many file descriptors open after your redirection. The code which you need is this.

    if (pid == 0)
{          /* for the child process:         */

    // function for redirection ( '<' , '>' )

    int fd0,fd1,i,in=0,out=0;
    char input[64],output[64];

    // finds where '<' or '>' occurs and make that argv[i] = NULL , to ensure that command wont't read that

    for(i=0;argv[i]!='\0';i++)
    {
        if(strcmp(argv[i],"<")==0)
        {        
            argv[i]=NULL;
            strcpy(input,argv[i+1]);
            in=2;           
        }               

        if(strcmp(argv[i],">")==0)
        {      
            argv[i]=NULL;
            strcpy(output,argv[i+1]);
            out=2;
        }         
    }

    //if '<' char was found in string inputted by user
    if(in)
    {   

        // fdo is file-descriptor
        int fd0;
        if ((fd0 = open(input, O_RDONLY, 0)) < 0) {
            perror("Couldn't open input file");
            exit(0);
        }           
        // dup2() copies content of fdo in input of preceeding file
        dup2(fd0, 0); // STDIN_FILENO here can be replaced by 0 

        close(fd0); // necessary
    }

    //if '>' char was found in string inputted by user 
    if (out)
    {

        int fd1 ;
        if ((fd1 = creat(output , 0644)) < 0) {
            perror("Couldn't open the output file");
            exit(0);
        }           

        dup2(fd1, STDOUT_FILENO); // 1 here can be replaced by STDOUT_FILENO
        close(fd1);
    }

    execvp(*argv, argv);
    perror("execvp");
    _exit(1);

    // another syntax
    /*      if (!(execvp(*argv, argv) >= 0)) {     // execute the command  
            printf("*** ERROR: exec failed\n");
            exit(1);
     */ 
}


    else if((pid) < 0)
    {     
        printf("fork() failed!\n");
        exit(1);
    }

    else {                                  /* for the parent:      */

        while (!(wait(&status) == pid)) ; // good coding to avoid race_conditions(errors) 
    }
}

1 Comment

No , the initial answer wasn't complete . It takes me a long time to figure out , what is "in" and "out" there ?? , I mean I wasn't know that weather they are some inbuilt syntax from some library or hard-coded previously. Thus @Matt I answered it with complete code.
4

Here's what's happening. After you call fork() there are two processes executing that are duplicates of the original process. The difference is in the return value of fork() which is stored in pid.

Then both processes (the shell and the child) redirect their stdin and stdout to the same files. I think you were trying to save the previous fd in current_out, but as Seth Robertson points out, this doesn't currently work, since the wrong file descriptor is being saved. The parent also restores its stdout, but not stdin.

You could fix this bug, but you can do better. You don't actually have to redirect parent's output, just the child's. So simply check pid first. Then there is also no need to restore any file descriptors.

2 Comments

I think that you are on the right track for what the asker was trying to do, except of course the dup2() happened BEFORE saving current_out instead of after. Doing it all in the child would of course make the question moot so is the best option.
@SethRobertson I missed that. I think I did some autocorrection in my head :) I'll update the answer.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.