3
\$\begingroup\$

I'm writing a shell in C for Linux and BSD. My code was unreadable before but I did a rewrite and now it is much more readable. The purpose of the code is to create a command pipeline and execute it.

int run_cmd(const char *cmd) {
    struct str_list *chunks = list_split(cmd, '|');
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    pipe->data = malloc(sizeof(char *));
    int i = 0;
    for (i = 0; i < chunks->pipes; i++) {
        pipe[i].data = malloc(sizeof(char *) * 3);
        for (int j = 0; j < 1; j++) {
            pipe[i].data[j] = strdup(chunks[i].argv[j]);
        }
    }
    pipe->size = chunks->pipes;
    int status = execute_pipeline(pipe);
    return status;
}

The above code makes the correct output and doesn't crash, but maybe it can be refactored even more. What was suggested was that I create structs:

struct str_list {
    char *name;
    int size;
    int pipes;
    char **argv;

};
struct pipeline {
    char *name;
    int size;
    char **data;
};

The helper functions: The first one takes a command and splits it first in pipelines and then into arguments and returns a struct. The second helper function is executing the pipeline.

/* TODO: modify str_split to do the copying of its input string if it needs to (e.g. if it uses strtok on it), and return a struct that has the number of "chunks" it split out and the list of chunks. */
struct str_list *list_split(char *a_str, const char a_delim) {
    char **result = 0;


    size_t count = 0;
    char *tmp = a_str;
    char *last_comma = 0;

    size_t count2 = 0;
    char *tmp2 = a_str;


    char delim[2];
    delim[0] = a_delim;
    delim[1] = 0;
    struct str_list *chunks = NULL;
    /* Count how many elements will be extracted. */
    while (*tmp) {
        if (a_delim == *tmp) {
            count++;
            last_comma = tmp;
        }
        tmp++;

    }

    /* Add space for trailing token. */
    count += last_comma < (a_str + strlen(a_str) - 1);

    count++;
    result = malloc(sizeof(char *) * count);
    chunks = malloc(count * sizeof *chunks);

    if (result == NULL) {
        printf("Error allocating memory!\n"); //print an error message
        return chunks;; //return with failure
    }

    if (result) {
        size_t idx = 0;
        char *token = strtok(a_str, delim);
        int i = 0;
        while (token) {
            assert(idx < count);
            *(result + idx++) = strdup(token); /* memory leak! how to free() */;
            token = strtok(0, delim);;
        }
        assert(idx == count - 1);
        *(result + idx) = 0;
    }
    chunks->argv = malloc(chunks->size * sizeof(char *) * 1);//alloc_argv((unsigned) chunks->size);
    int i = 0;

    for (i = 0; i < count - 1; i++) {
        tmp2 = result[i];
        chunks[i].size = 0;
        while (*tmp2) {
            if (' ' == *tmp2) {
                count2++;
            }
            tmp2++;
        }
        //chunks[i].size = (unsigned) count2 + 1;
        int k = 0;
        char *token = strtok(result[i], " ");
        chunks[i].argv = alloc_argv(BUFFER_SIZE);
        while (token) {
            chunks[i].argv[k] = malloc(sizeof(char *));
            chunks[i].argv[k] = strdup(token);
            token = strtok(0, " ");
            k++;
        }
        chunks[i].size = k;
    }
    chunks->pipes = i;
    return chunks;
}





int execute_pipeline(struct pipeline *pipe) {
    int status = 0;
    pid_t pid;
    pid = fork();
    if (pid < 0) {
         perror("fork failed");
         return -1;
    }
    /* If we are the child process, then go execute the string.*/
    if (pid == 0) {
        /* spawn(cmd);*/
        fork_pipeline(pipe->size, pipe);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */

    while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));
    if (pid < 0) {
        fprintf(stderr, "Error from waitpid: %s", strerror(errno));

        return -1;
    }
    if (WIFSIGNALED(status)) {
        fprintf(stderr, "pid %ld: killed by signal %d\n",
                (long) pid, WTERMSIG(status));


        return -1;
    }


    return WEXITSTATUS(status);
    return 1;

}


/* Helper function that forks pipes */
void fork_pipeline(int n, struct pipeline *pipel) {
    int i;
    int in = 0;
    int fd[2];
    for (i = 0; i < n - 1; ++i) {

        if (pipe(fd) == -1) {
            err_syserr("Failed creating pipe");
        }

        spawn_proc2(in, fd[1], pipel + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0) {
        err_syserr("dup2() failed on stdin for %s: ", pipel[i].data[0]);
    }
    /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd[i].argv[0]);*/
    fprintf(stderr, "\n");

    execvp(pipel[i].data[0], pipel[i].data);
    err_syserr("failed to execute %s: ", pipel[i].data[0]);
}


/* Helper function that spawns processes */
int spawn_proc2(int in, int out, struct pipeline *cmd) {
    pid_t pid;
    fflush(NULL);
    pid = fork();
    if (pid == 0) {
        if (in != 0) {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->data[0]);
            close(in);
        }
        if (out != 1) {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->data[0]);
            close(out);
        }
        /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd->argv[0]);*/
        fprintf(stderr, "[%d]\n", (int) getpid());
        execvp(cmd->data[0], cmd->data);
        err_syserr("failed to execute %s: ", cmd->data[0]);
    }
    else if (pid < 0) {
        err_syserr("fork failed: ");
    } else {
        /* printf("** we are the parent ***"); */
    }
    return pid;
}

The code works:

$ ls -al|cat|grep -E CMake
ls -al|cat|grep -E CMake
-rw-rw-r-- 1 dac dac  12180 maj 11 12:40 CMakeCache.txt
drwxrwxr-x 6 dac dac   4096 maj 12 12:03 CMakeFiles
\$\endgroup\$
3
\$\begingroup\$

Just looking at list_split:

  1. There's no specification for this function. Without a specification, it's hard to review it: how am I to know if the code is correct unless I know what it's supposed to do?

  2. There's no specifications for the str_list type. What is str_list.size the size of? What does str_list.pipes count? What is str_list.name? (It doesn't seem to be used anywhere.)

  3. Error checking is far from complete: the code calls functions (malloc and strdup) that might fail, but in most cases does not check the results.

  4. There's no way for the caller to tell if str_list succeeded or failed. For example, if the allocation of result fails, it returns chunks, which might, for all we know, be a valid pointer. It would be better to arrange the code so that there is a return value indicating success or failure.

  5. Memory is allocated but never freed, which will cause the process size to grow without bound, eventually leading to some kind of failure.

  6. The splitting just proceeds character-by-character without regard to syntax. This means that a command cannot contain a vertical bar, and an argument cannot contain a space. But real shells have quoting, so that we can write pipelines like:

    echo "This argument has | and spaces" | grep -c "|"
    

    Obviously you're just getting started and want to keep things simple, but it would be a good idea to think ahead about how you are going to solve this problem.

  7. Similarly, your implementation assumes that a pipeline is a list of commands, each of which is a list of arguments. But real shells have more structure than this: they have compound commands:

    { echo 1; echo 2; echo 3; } | while read X; do printf "%02d" $X; done
    

    Again, it's worth thinking ahead about how you are going to implement compound commands.

\$\endgroup\$
  • \$\begingroup\$ Yes, I'm just getting started and I'm trying to look ahead. I'm working on how to execute the while keyword, but it is difficult to do that with a grammar. I am learning the lemon grammar. I also created a repository for the code. Currently I use a manually coded parser but I am going to try and use a lemon-parser grammar if I can learn how to do it. \$\endgroup\$ – Niklas May 12 '16 at 23:17
  • \$\begingroup\$ This was my problem: What is str_list.size the size of? What does str_list.pipes count? I keep two variables, one is the size of the entire command-line and the other is the size / length of a single command within a pipeline that consists of several commands. I'm trying to refactor it so that they are better. \$\endgroup\$ – Niklas May 27 '16 at 22:58
  • \$\begingroup\$ The example echo "This argument has | and spaces" | grep -c "|" is very good and I have been thinking about it a lot, I have asked about it and there is a solution in this question programmers.stackexchange.com/questions/316767/… That is an algorithm that will determine if a char is between quotes. \$\endgroup\$ – Niklas May 27 '16 at 23:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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