patterncMinor
run command pipeline in C
Viewed 0 times
commandpipelinerun
Problem
This code parses and tokenizes my shell language command interpreter. I will try and make it 2 functions, one that is the tokenizer and the other that is the rest of the function. Can you find improvements that I can make besides breaking up the function into 2 smaller functions, with a helper function that is the tokenizer? Perhaps it can be done into 3 functions where the 3rd helper function can be the copying code for copying the
Or even 4 different functions since this one can also be broken out (that strips leading whitespace)
The whole function is:
```
static int runCmd(const char *cmd) {
const char *cp;
pid_t pid;
int status;
struct command shellcommand[4];
const char **argv;
int argc;
int j;
char *params[100];
int i = 0;
char *token;
char **new_argv;
for (cp = cmd; *cp; cp++) {
if ((cp >= 'a') && (cp = 'A') && (*cp <= 'Z'))
continue;
if (isDecimal(*cp))
continue;
if (isBlank(*cp))
continue;
if ((cp == '.') || (cp == '/') || (*cp == '-') ||
(cp == '+') || (cp == '=') || (*cp == '_') ||
(cp == ':') || (cp == ',') || (*cp == '\'') ||
(*cp == '"')) {
continue;
}
}
makeArgs(cmd, &argc, &argv);
token = strtok(cmd, "|");
i = 0;
j = 0;
params[0] = NULL;
while (token != NULL) {
char * tmp = token;
int x;
while (*tmp == ' ') {
*tmp++;
}
x = 0;
while (*tmp != NULL) {
token[x++] = *tmp++;
}
token[x] = '\0';
makeArgs(token, &argc, &argv);
/ Will copy argc characters /
new_argv = malloc((argc + 1) sizeof new_ar
argv variable. Or even 4 different functions since this one can also be broken out (that strips leading whitespace)
char * tmp = token;
int x;
while (*tmp == ' ') {
*tmp++;
}
x = 0;
while (*tmp != NULL) {
token[x++] = *tmp++;
}
token[x] = '\0';The whole function is:
```
static int runCmd(const char *cmd) {
const char *cp;
pid_t pid;
int status;
struct command shellcommand[4];
const char **argv;
int argc;
int j;
char *params[100];
int i = 0;
char *token;
char **new_argv;
for (cp = cmd; *cp; cp++) {
if ((cp >= 'a') && (cp = 'A') && (*cp <= 'Z'))
continue;
if (isDecimal(*cp))
continue;
if (isBlank(*cp))
continue;
if ((cp == '.') || (cp == '/') || (*cp == '-') ||
(cp == '+') || (cp == '=') || (*cp == '_') ||
(cp == ':') || (cp == ',') || (*cp == '\'') ||
(*cp == '"')) {
continue;
}
}
makeArgs(cmd, &argc, &argv);
token = strtok(cmd, "|");
i = 0;
j = 0;
params[0] = NULL;
while (token != NULL) {
char * tmp = token;
int x;
while (*tmp == ' ') {
*tmp++;
}
x = 0;
while (*tmp != NULL) {
token[x++] = *tmp++;
}
token[x] = '\0';
makeArgs(token, &argc, &argv);
/ Will copy argc characters /
new_argv = malloc((argc + 1) sizeof new_ar
Solution
Memory leaks
Since this is part of a shell and can therefore be considered an extension of the OS which should not stop at any point, you have a memory leak problem. Unlike Java and Python there is no garbage collection, you have to manage the memory on your own. Any
Use the standard library when possible
It appears you have written your own functions
If you're on a Linux system try
Your for loop
becomes simply
Since this is part of a shell and can therefore be considered an extension of the OS which should not stop at any point, you have a memory leak problem. Unlike Java and Python there is no garbage collection, you have to manage the memory on your own. Any
malloc() calls you make either explicitly or implicitly with functions such as strdup() need corresponding calls to free(). You need to clean up new_argv. This will need to be a loop that calls free() for each of the arguments in new_argv and then free() new_argv itself. See this question on Programmers SE.Use the standard library when possible
It appears you have written your own functions
isDecimal(), isBlank() rather than using the standardized functions such as isalpha() (defined in the for loop), isspace() (finds all white space, not just blank) and strdup(). There are a number of character functions in ctype.h that can help, and string.h will provide the definitions of strdup(), strcpy(), strcmp() and strlen() which will also help.#include
#include If you're on a Linux system try
man ctype and man strdup.Your for loop
for (int i = 0; i < argc; ++i) {
size_t length = strlen(argv[i]) + 1;
new_argv[i] = malloc(length);
memcpy(new_argv[i], argv[i], length);
}becomes simply
for (int i = 0; i < argc; i++) {
new_argv[i] = strdup(argv[i]);
}Code Snippets
#include <ctype.h>
#include <string.h>for (int i = 0; i < argc; ++i) {
size_t length = strlen(argv[i]) + 1;
new_argv[i] = malloc(length);
memcpy(new_argv[i], argv[i], length);
}for (int i = 0; i < argc; i++) {
new_argv[i] = strdup(argv[i]);
}Context
StackExchange Code Review Q#126489, answer score: 5
Revisions (0)
No revisions yet.