run command pipeline in C

Question

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 <= 'z'))
            continue;

        if ((*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_argv);
        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);
        }
        new_argv[argc] = NULL;
        shellcommand[i].argv = new_argv;
        i++;
        token = strtok(NULL, "|");
    }
    pid = fork();
    if (pid < 0) {
        perror("fork failed");
        return -1;
    }
    /*
     * If we are the child process, then go execute the program.
     */
    if (pid == 0) {
        /* spawn(cmd);*/
        fork_pipes(i, shellcommand);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */
    status = 0;
    intCrlf = FALSE;

    while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));

    intCrlf = TRUE;

    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);
}

Update

I have made a refactoring to make it smaller. I have broken out the trim leading whitespace to a function.

char * trimstring(char * token, int operation) {
    char * tmp = token;
    int x;
    while (*tmp == ' ') {
        *tmp++;
    }
    x = 0;
    while (*tmp != NULL) {
        token[x++] = *tmp++;
    }
    token[x] = '\0';
    return token;
}


static int runCmd(const char *cmd) {
    const char *cp;
    pid_t pid;
    int status;
    struct command shellcommand[4];
    const char **argv;
    int argc;
    int i = 0;
    char *token;
    char **new_argv;
    for (cp = cmd; *cp; cp++) {
        if ((*cp >= 'a') && (*cp <= 'z'))
            continue;

        if ((*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((char *) cmd, "|");
    i = 0;
    while (token != NULL) {
        token  = trimstring(token, 0);
        makeArgs(token, &argc, &argv);
        /* Will copy argc characters */
        new_argv = malloc((argc + 1) * sizeof *new_argv);
        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);
        }
        new_argv[argc] = NULL;
        shellcommand[i].argv = new_argv;
        i++;
        token = strtok(NULL, "|");
    }
    pid = fork();
    if (pid < 0) {
        perror("fork failed");
        return -1;
    }
    /* If we are the child process, then go execute the program.*/
    if (pid == 0) {
        /* spawn(cmd);*/
        fork_pipes(i, shellcommand);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */
    status = 0;
    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);
}

Answer 1

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 <ctype.h>
#include <string.h>

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]);
}