2

I am writing a shell and currently implementing the pipe process. I am able to run "ls -l | wc", but when running "ls -l|wc" the process fails.

I have tried debugging with gdb but unable to make anything of it as they both seem as they are equal to the same values (after being parsed).

When running ls -l | wc and having a breakpoint on the parseCmdWithPipe function and printing out the locals I get:

argOne = {0x7fffffffdad0 "ls", 0x7fffffffdad3 "-l", 0x0}
argTwo = {0x7fffffffdad8 "wc", 0x0}

When running ls -l|wc and having a breakpoint on the parseCmdWithPipe function and printing out the locals I get:

argOne = {0x7fffffffd9b0 "ls", 0x7fffffffd9b3 "-l", 0x0}
argTwo = {0x7fffffffd9b8 "wc", 0x0}

The code I use to parse and use execvp (second function)

/* parse user input from fget */
int parseLine(char *line, char **argv) {
    line[strlen(line) - 1] = '\0';
    //This section tries to figure out if we have the case
    //"ls -l|wc", if it does it makes a new string of "ls -l | wc"
    int cmdType = 1; 
    int size = sizeof(line);
    char tempLine[size + 3];
    bool pipeSeen = false;
    for (int i = 0; i < size; i++) {
        if (line[i] == '|' && line[i - 1] != ' ') {
            tempLine[i]     = ' ';
            tempLine[i + 1] = '|';
            tempLine[i + 2] = ' ';
            pipeSeen = true;
        } else {
            if (pipeSeen == true)
                tempLine[i + 2] = line[i];
            else
                tempLine[i] = line[i];
        }
    }

    if (pipeSeen == true) {
        tempLine[size + 2] = '\0';
        line = tempLine; // change "ls -l|ls to ls -l | wc"
    }
    /* parse line and store it in argv */
    while (*line) {
        while (*line == ' ')
            *line++ = '\0';

        //Figure out what type of command we may have
        if (*line == '<')
            cmdType = (cmdType == 3) ? 5 : 2;
        else if (*line == '>' )
            cmdType = (cmdType == 2) ? 5 : 3;
        else if (*line == '|')
            cmdType = 4;

        *argv++ = line;
        while (*line != '\0' && *line != ' ' && *line != '\t' && *line != '\n')
            line++;
    }
    *argv = '\0';
    return cmdType;
}

void parseCmdWithPipe(char **argv) {
    int size = getCmdSize(argv);
    char val[1];
    int pipePosition = 0;
    for (int i = 0; i < size; i++) {
        strcpy(val, argv[i]);
        if (val[0] == '|') {
            pipePosition = i;
            break;
        }
    }

    char *argOne[pipePosition + 1];
    for (int i = 0; i < pipePosition; i++) {
        argOne[i] = argv[i];
    }
    argOne[pipePosition] = '\0';

    char *argTwo[size - pipePosition];
    for (int i = 0; i < (size - pipePosition); i++) {
        argTwo[i] = argv[i + pipePosition + 1];
    }
    argTwo[size - pipePosition - 1] = '\0';

    int pfd[2], pid;
    if (pipe(pfd) == 1)
        syserror("Could Not create a pipe");

    switch (pid = fork()) {
      case -1:
        syserror( "Fork Failed" );
      case 0:
        if (dup2(pfd[0], 0) == -1|| close (pfd[0]) == -1 || close (pfd[1]) == -1)
            syserror("Failed to run dup2 or close pfd in second child");
        execvp(argTwo[0], argTwo);
        syserror("execvp unsuccessful in 2nd child");
    }
     ....

I expected "ls -l|wc" to give me the same result "ls -l | wc" gives me which is 

11      92     597

but the actual output was 

execvp unsuccessful in 2nd child
(No such file or directory)
chqrlie
  • 131,814
  • 10
  • 121
  • 189
Continuum
  • 67
  • 1
  • 9
  • 2
    `int size = sizeof(line)` doesn't do what you think. `strcpy(val, argv[i]);` doesn't end well when `val` is defined as `char val[1];`. – Raymond Chen Feb 18 '19 at 23:59
  • note that [piping `ls` output to other tools is incorrect](https://unix.stackexchange.com/q/128985/44425). There are better ways to count number of files [Is there a bash command which counts files?](https://stackoverflow.com/q/11307257/995714), [What's the most resource efficient way to count how many files are in a directory?](https://unix.stackexchange.com/q/90106/44425) – phuclv Feb 19 '19 at 02:22
  • @phuclv The point of this exercise is ***not*** to count the number of files. – user253751 Feb 19 '19 at 02:43

1 Answers1

1

The main problem is int size = sizeof(line); which does not compute the size nor the length of the string pointed to by line, but merely evaluates to the size of a char *.

You should write int size = strlen(line); and probably not call this variable size, but len instead.

The code is too complicated, you should not try to insert spaces around the | symbol, by the way, you fail to do the same for < and >.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • I changed sizeof to strlen! However it didn't do much as gdb still produces the same results. I definitely agree its a bit overcomplicated. Do you have any suggestions on how to improve it? – Continuum Feb 19 '19 at 01:38
  • @Continuum: yes I would, but it is 2:40am here and I have to get some sleep. – chqrlie Feb 19 '19 at 01:42