-1

I am writing my own shell. I want to implement redirection( > and >>). For that I have used dup2() system call. But if a command I enter has redirection, another command follows the previous redirection even if I don't use > or >> in it. Am I missing to close previous file descriptor? Also even though I use >, the output is always appended to the file.

Following is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/wait.h>
#include <setjmp.h>
#include <signal.h>
#include <fcntl.h>
#include <stdbool.h>

#define MAXLINE 259
#define PROMPT "> "
#define MAX_ARG_LIST    200
//-Wno-write-strings to g++ compiler to supress warnings

extern char **environ;

#define MAX_CMD_SIZE    50
#define SEARCH_FOR_CMD  -1
typedef void (*buildInFunc) (char **);
typedef struct {
    char cmd[MAX_CMD_SIZE];
    buildInFunc func;
} builtInCmd;

// built-in commands
void execExit(char *cmd[]);
void execCd(char *cmd[]);
builtInCmd builtInCmds[] = {
        {"exit",    execExit  },
        {"cd",  execCd }
};
int builtInCnt = sizeof(builtInCmds)/sizeof(builtInCmd);
int isBuiltIn(char *cmd);

void execBuiltIn(int i, char *cmd[]);

// capture SIG_INT and recover
sigjmp_buf ctrlc_buf;
void ctrl_hndlr(int signo) {
    siglongjmp(ctrlc_buf, 1);
}

int main(int argc, char *argv[]) {

    char line[MAXLINE];
    pid_t childPID;
    int argn; char *args[MAX_ARG_LIST];
    int cmdn;
        char *in_file,*out_file;
        char *gt=">";       //truncate redirection char pointer
        char *gtgt=">>";    //append redirection char pointer        
        int in_fd,out_fd;   //
        bool out_fd_present=false;  //checks if > or >> string is present

    // setup SIG_INT handler
    if (signal(SIGINT, ctrl_hndlr) == SIG_ERR)
        fputs("ERROR: failed to register interrupts in kernel.\n", stderr);

    // setup longjmp buffer
    while (sigsetjmp(ctrlc_buf, 1) != 0) ;

    for(;;) {
    // prompt and get commandline
        fputs(PROMPT, stdout);
        fgets(line, MAXLINE, stdin);
        if (feof(stdin)) break; // exit on end of input

    // process commandline
        if (line[strlen(line)-1] == '\n')
            line[strlen(line)-1] = '\0';
        // build argument list
        args[argn=0]=strtok(line, " \t");
                while(args[argn]!=NULL && argn<MAX_ARG_LIST){

                        args[++argn]=strtok(NULL," \t");
                        //if append >> redirection present
                        if(strcmp( args[argn-1],gtgt ) == 0){
                            out_fd_present=true;
                            out_file=args[argn];
                            argn=argn-2;
                            out_fd = open(out_file, O_WRONLY | O_APPEND | O_CREAT,S_IRWXG | S_IRWXO | S_IRWXU);
                            //printf("\n >> found\n");                            
                        }
                        //if trncate > redirection present
                        else if(strcmp( args[argn-1],gt ) == 0){
                            out_fd_present=true;
                            out_file=args[argn];
                            argn=argn-2;
                            out_fd = open(out_file, O_WRONLY | O_CREAT | O_TRUNC, S_IRWXG | S_IRWXO | S_IRWXU);
                            //printf("\n > found\n");
                        }
                        printf("args[%d]=%s\n",argn-1,args[argn-1]);
                }


    // execute commandline
    if ((cmdn = isBuiltIn(args[0]))>-1) {
        execBuiltIn(cmdn, args);
    } else {
        childPID = fork();
                int save_out;
        if (childPID == 0) {
                    if(out_fd_present){
                        int a;
                        save_out = dup(STDOUT_FILENO);
                        dup2(out_fd,1);
                        close(out_fd);                        
                    }
                        execvp(args[0], args);                        
            fputs("ERROR: can't execute command.\n", stderr);
            _exit(1);
        } else {
            waitpid(childPID, NULL, 0);
        }
    }

    // cleanup
                fflush(stderr);
                fflush(stdout);
    }

    return 0;
}

int isBuiltIn(char *cmd) {
    int i = 0;        
    while (i<builtInCnt) {
        if (strcmp(cmd,builtInCmds[i].cmd)==0)
            break;
        ++i;
    }
    return i<builtInCnt?i:-1;
}

void execBuiltIn(int i, char *cmd[]) {
    if (i == SEARCH_FOR_CMD)
        i = isBuiltIn(cmd[0]);
    if (i >= 0 && i < builtInCnt)
        builtInCmds[i].func(cmd);
    else
        fprintf(stderr, "ERROR: unknown built-in command\n");
}

void execExit(char *cmd[]) {        
    exit(0);
}
void execCd(char *cmd[]){
    chdir(cmd[1]);
}

closed as off-topic by Gilles, techraf, Jeff Schaller, HalosGhost, Eric Renouf Nov 28 '16 at 1:36

  • This question does not appear to be about Unix or Linux within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

1

You have a simple logic bug: while you get out_fd_present to true when you detect > or >>, you never reset it to false (e.g. at the top of the loop).

While I guess that your "shell" is a toy shell, I have to mention that you have many other problems in there too. For example, you leak out_fd in the parent process and save_out (what is that for?) in the child. You have a bug where you may call strcmp on a NULL string. And another one for chdir. Your syntax and functionality is also different from what a shell "should" do (position of redirection operators, umask, spacing and tokenization, etc...). Not to mention inconsistent style (ugly camel case and snake case both used). To implement a shell, even a toy shell, you should probably start with a good design first.

  • CamelCase? Ugly? – DepressedDaniel Nov 27 '16 at 23:27
  • @DepressedDaniel I know that's an opinion of mine, but I couldn't help inserting that adjective. Personally, I have a great deal more trouble reading theseWords than reading these_words. Moreover, the former is very bizarre to me because if you change theseWords to moreOfTheseWords, the "t" suddenly became uppercase for a reason that is very hard to justify, that is, the case of the existing words shouldn't change just because you add more words in front. For that reason, ThisKindOfCamelCase (e.g. Python class names) is sort of acceptable to me but thisKindOfCamelCase is not. – Celada Nov 27 '16 at 23:39
  • @Celada Thanks. That solved 75% of my problems. chdir is used to implement build-in so as to change the current directory of shell. I have assumed cd will have one argument following it. Even in this case NULL error may occur madam? – Sulav Timsina Nov 28 '16 at 0:45
  • If I'm reading the code correctly, chdir will get a NULL argument if the used entered no argument (i.e. the full command line is just cd alone). You have to protect against incorrect user input and give the user an error message if they supplied the wrong number of arguments :-) – Celada Nov 28 '16 at 13:53

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