patterncMinor
Running shell commands in a pipeline
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.
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:
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;
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
-
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
-
Error checking is far from complete: the code calls functions (
-
There's no way for the caller to tell if
-
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:
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:
Again, it's worth thinking ahead about how you are going to implement compound commands.
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; doneAgain, 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; doneContext
StackExchange Code Review Q#128149, answer score: 3
Revisions (0)
No revisions yet.