HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Running shell commands in a pipeline

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
commandsrunningpipelineshell

Problem

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 pipes; i++) {
        pipe[i].data = malloc(sizeof(char *) * 3);
        for (int j = 0; j 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 argv = malloc(chunks->size sizeof(char ) * 1);//alloc_argv((unsigned) chunks->size);
int i = 0;

for (i = 0; i pipes = i;

Solution

Just looking at list_split:

-
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?

-
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.)

-
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.

-
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.

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

-
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.

-
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.

Code Snippets

echo "This argument has | and spaces" | grep -c "|"
{ echo 1; echo 2; echo 3; } | while read X; do printf "%02d" $X; done

Context

StackExchange Code Review Q#128149, answer score: 3

Revisions (0)

No revisions yet.