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

run command pipeline in C

Submitted by: @import:stackexchange-codereview··
0
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 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 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.