Shell command line program

Question

I have refactored a for loop into a function. I'm still a beginner at using C utility functions such as strtok, strdup, malloc andstrcpy. The program makes the correct output for another program (my program is a shell command language interpretor) showing that I have 1800 lines of code:

 $ cat *.c *.h|wc -l
 {cat} {errors.c} {main.c} {util.c} {CommandEntry.h} {do.h} {errors.h} {openshell.h} {|} {wc} {-l} {|}
p[0][0] cat
p[0][1] errors.c
p[0][2] main.c
p[0][3] util.c
p[0][4] CommandEntry.h
p[0][5] do.h
p[0][6] errors.h
p[0][7] openshell.h
p[1][0] wc
p[1][1] -l

1833

You can see the strategy I used was to use a matrix A[i][j] for a command language program where element [i][j] is program i, argument j.

The functions I have refactored are:

void write_command(int *w, char *argv[], char *string[]) {
    for (int r = 0; argv[r] != NULL; r++) {
        string[r] = strdup(argv[r]);
    }
}

void write_argument(int *argc, char *argv[], char *string[]) {
    int j = 0;
    for (j = 0; j < *argc; j++) {
        string[j] = strdup(argv[j]);
    }
}

The large function that calls those functions is:

static int runCmd(const char *cmd) {
    const char *cp;
    pid_t pid;
    int status;
    struct command structcommand[15];
    char **argv = 0;
    int argc = 1;
    bool pipe = false;
    char *string[z][z];
    char *pString3[40];
    char *pString2[40];
    int n = 0;
    char **ptr1;
    char string1[z];
    bool keep = false;
    char *pString1[z];
    char *pString[z];
    *pString1 = "\0";
    *pString = "\0";
    char *temp = {'\0'};
    int w = 0;
    bool b = false;
    int j = 0;
    int i;
    int p = 0;
    char **ptr;
    char *tmpchar;
    char *cmdtmp;
    bool b1 = false;
    int y = 0;
    i = 0;
    int h = 0;
    nullterminate(string);
    if (cmd) {
        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;
            }
        }
    }
    if (cmd) {
        cmdtmp = malloc(sizeof(char *) * strlen(cmd) + 1);
        strcpy(cmdtmp, cmd);
        tmpchar = malloc(sizeof(char *) * strlen(cmd) + 1);
        if (tmpchar == NULL) {
            printf("Error allocating memory!\n"); /* print an error message */
            return 1; /* return with failure */
        }
        strcpy(tmpchar, cmd);
        ptr1 = str_split(pString3, cmdtmp, '|');
        if (strstr(cmd, "|") == NULL) {         /* not a pipeline */

            makeArgs(cmd, &argc, (const char ***) &argv, pipe, 0, 0);
            write_argument(&argc, argv, string[0]);
            n++;
        }
        else {
            for (i = 0; *(ptr1 + i); i++) { /* tokenize the input string for each pipeline*/
                n++; /* save number of pipelines */
                int e = 0; /* a counter */
                *pString = "\0"; /* should malloc and free this? */
                strcpy(string1, *(ptr1 + i));
                if ((string1[0] != '\0') && !isspace(string1[0])) { /* this is neither the end nor a new argument */
                    ptr = str_split(pString2, *(&string1), ' '); /* split the string at the arguments */
                    h = 0;
                    for (j = 0; *(ptr + j); j++) { /* step through the arguments */
                        /* the pipeline is in cmdtmp and the argument/program is in ptr[i] */
                        if (ptr + j && !b && strstr(*(ptr + j), "'")) {
                            b = true;
                            strcpy(temp, *(ptr + j));
                            if (y < 1) {
                                y++;
                            }
                        }
                        while (b) {
                            if (*(ptr + j) && strstr(*(ptr + j), "'")) { /* end of quote */
                                b = false;
                                if (y < 1) {
                                    string[i][j] = strcpy(temp, *(ptr + j));
                                }
                                y = 0;
                            }
                            else if (*(ptr + j)) { /* read until end of quote */
                                string[i][j] = temp;
                                continue;
                            } else {
                                b = false;
                                break;
                            }
                        }
                        if (ptr + j) {
                            if (*(ptr + j)[0] == '{') {
                                keep = true;
                            }
                            if (testFn(*(ptr + j))) { /* test for last char */
                                string[i][j - p] = concat(*pString1, *(ptr + j));
                                keep = false;
                                free(*pString1);
                                goto mylabel;
                            }
                            if (keep) {
                                *pString1 = concat(*pString1, *(ptr + j));
                                *pString1 = concat(*pString1, " ");
                                p++;
                            } else {
                                b1 = false;
                                int q = j;
                                for (e = 0; *(ptr + q + e); e++) { /* step through the string */
                                    b1 = true;
                                    if (*(ptr + e + q)) {
                                        *pString = concat(*pString, *(ptr + e + q));
                                        *pString = concat(*pString, " ");
                                    }
                                    j = e;
                                }
                                if (makeArgs(*pString, &argc, (const char ***) &argv, pipe, i, h)) {
                                    write_command(&w, argv, string[w]);
                                    w++;

                                } else {
                                    if (!b1) { /* no args (?) */
                                        for (int r = 0; argv[r] != NULL; r++) {
                                            string[i][r] = argv[r];
                                        }
                                    }
                                }
                            }
                        }
                    }
                    mylabel:
                    free(ptr);
                    dump_argv((const char *) "d", argc, argv);
                }
            }
            free(ptr1);
            free(cmdtmp);
            free(tmpchar);
        }
        for (i = 0; i < n; i++) {
            for (j = 0; DEBUG && string[i][j] != NULL; j++) {
                if (i == 0 && j == 0) printf("\n");
                printf("p[%d][%d] %s\n", i, j, string[i][j]);
            }
            structcommand[i].argv = string[i];
        }
        fflush(NULL);
        pid = fork();
        if (pid < 0) {
            perror("fork failed");
            return -1;
        }
        /* If we are the child process, then go execute the string.*/
        if (pid == 0) {
            /* spawn(cmd);*/
            fork_pipes(n, structcommand);
        }
        /*
         * 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);


}

My strategy is to factor out all the loops and make every loop a function and then the code will be more readable (hopefully). I'm also interested if you have a refactoring strategy for my case.

Answer 1

You really need to reduce the scope of all your variables, and give them proper names. As it is, your code is impossible to follow, it's super hard to guess the type of anything, treacherous to determine whether it's been initialized properly or not, or whether it needs to be cleaned up. Discovering unused or redundant variables is pretty much impossible too.

And this penalizes you for refactoring: since all your variable declarations are bunched up at the top with opaque names, potential function boundaries are super hard to figure out. You don't know which parts of your code need what variables, what side-effects need to be visible to what blocks, etc.

Even your two refactored functions raise questions.

void write_command(int *w, char *argv[], char *string[]) {
    for (int r = 0; argv[r] != NULL; r++) {
        string[r] = strdup(argv[r]);
    }
}

What is w? It's not used, why have it as an argument? write_* is not a good prefix for something that duplicates something else. dup_* or copy_* would be more understandable. argv is sort of understood in C to be an args list, so that's ok. But string for a variable name is terrible. Try this:

void copy_args(char *source[], char *target[]) { /* ... */ }

The body is ok, but why r as a loop variable? i is the "default" unless you want to give it a meaningful name. r isn't meaningful, so it is suspicious.

void write_argument(int *argc, char *argv[], char *string[]) {
    int j = 0;
    for (j = 0; j < *argc; j++) {
        string[j] = strdup(argv[j]);
    }
}

Again, wrong prefix name for something that copies/duplicates. Same remark for the argument names. Now why is argc passed by pointer? It is only read in the function. And why declare j outside the loop? That extends its scope for no good reason. Plus you end up initializing it to zero twice - the compiler will optimize that out but why do it at all?

void copy_first_args(int n, char *source[], char *target[]) {
    for (int i = 0; i < n; i++) {
        target[i] = strdup(source[i]);
    }
}

---

The following needs to die.

const char *cp;
pid_t pid;
int status;
struct command structcommand[15];
char **argv = 0;
int argc = 1;
bool pipe = false;
char *string[z][z];
char *pString3[40];
char *pString2[40];
int n = 0;
char **ptr1;
char string1[z];
bool keep = false;
char *pString1[z];
char *pString[z];
*pString1 = "\0";
*pString = "\0";
char *temp = {'\0'};
int w = 0;
bool b = false;
int j = 0;
int i;
int p = 0;
char **ptr;
char *tmpchar;
char *cmdtmp;
bool b1 = false;
int y = 0;
i = 0;
int h = 0;
nullterminate(string);

There is no consistency at all, it is a jumbled mess of inscrutable names, random initializations, perplexing assignments and a scary function call - nullterminate takes a 2D array of pointers, with no size indication and you give it an uninitialized array? Sounds like a bug waiting to happen.

Looking at the toplevel flow of your function, you don't need a single one of those declared at the top. Except status. But there's a bug there. If !cmd, your function will return something based on status that has not been initialized => undefined behavior. That's exactly the sort of problem this "style" of declaring everything at the top and not initializing them consistently brings.

Also z appears to be a global name (variable? constant? macro? who knowns). That's a terrible name for a global. Find a better, longer one. This is especially important for globals.

---

Structure of that function:

int runCmd(const char *cmd) {
  /* decls */
  if (cmd) { /* 1. chunk of code that doesn't do anything */ }
  if (cmd) { /* 2. unreasonably large chunk of code */ }
  return stuff;
}

Problems:

• As noted before, if !cmd you return junk
• Two different if blocks in a row with the same condition is suspicious, only makes sense if the first chunk could affect the conditional. But it doesn't.
• One of the blocks doesn't actually do anything.

You'll win one level of nesting and a tiny bit less confusion for the readers by laying it out like this instead:

int runCmd(const char *cmd) {
  if (!cmd) { return error; /* or assert */ }
  /* code */
}

---

I'll just look at the next few lines of code:

cmdtmp = malloc(sizeof(char *) * strlen(cmd) + 1);
strcpy(cmdtmp, cmd);

You know about strdup, use it. The allocation above will be about eight times too large on usual 64bit platforms - what you should have there is sizeof(char), not char*, or just strlen(cmd)+1 since by definition sizeof(char) == 1.

tmpchar = malloc(sizeof(char *) * strlen(cmd) + 1);
if (tmpchar == NULL) {
    printf("Error allocating memory!\n"); /* print an error message */
    return 1; /* return with failure */
}
strcpy(tmpchar, cmd);

Same remark. Except why did you check this allocation but not the previous one? Be consistent. Either ignore all allocation failures or deal with all of them consistently.

ptr1 = str_split(pString3, cmdtmp, '|');
if (strstr(cmd, "|") == NULL) {         /* not a pipeline */

    makeArgs(cmd, &argc, (const char ***) &argv, pipe, 0, 0);
    write_argument(&argc, argv, string[0]);
    n++;
} else { // ...

So your code duplicates cmd twice, splits one copy based on a delimiter, then checks if the delimiter was present in the first place and does stuff that doesn't need either copy or the result of the split if it wasn't. pipe appears out of nowhere, let's hope it's been initialized. This is all very suspicious.

So a quick grep:

$ grep tmpchar t.c
    char *tmpchar;
        tmpchar = malloc(sizeof(char *) * strlen(cmd) + 1);
        if (tmpchar == NULL) {
        strcpy(tmpchar, cmd);
            free(tmpchar);
$ grep cmdtmp t.c
    char *cmdtmp;
        cmdtmp = malloc(sizeof(char *) * strlen(cmd) + 1);
        strcpy(cmdtmp, cmd);
        ptr1 = str_split(pString3, cmdtmp, '|');
                        /* the pipeline is in cmdtmp and the argument/program is in ptr[i] */
            free(cmdtmp);
$ grep pipe t.c
    bool pipe = false;
        if (strstr(cmd, "|") == NULL) {         /* not a pipeline */
            makeArgs(cmd, &argc, (const char ***) &argv, pipe, 0, 0);
            for (i = 0; *(ptr1 + i); i++) { /* tokenize the input string for each pipeline*/
                n++; /* save number of pipelines */
                        /* the pipeline is in cmdtmp and the argument/program is in ptr[i] */
                                if (makeArgs(*pString, &argc, (const char ***) &argv, pipe, i, h)) {
            fork_pipes(n, structcommand);

And here's what transpires:

• tmpchar is in fact not used.
• cmdtmp is only used as the source of the split.
• Both are leaked if there was no separator (notice the extra indent before the free).
• pipe is a boolean that is always false.

Here are your first steps in being able to refactor this:

• get rid of tmpchar
• get rid of cmdtmp
• get rid of pipe
• rename makeArgs to make_args or change the other ones to use funkyCase - pick one style and stick to it.
• 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.
• create a struct for the individual commands and arguments. It should have something like the "executable" name, number of args and arg list. Create a few functions to create such a struct, free it, and add to the argument list.

Then your function can start to look like this:

int run_cmd(const char *cmd) {
  assert(cmd);
  struct str_list *chunks = str_split(cmd, '|');
  struct pipeline *pipe = alloc_pipeline(chunks->size);
  for (int i=0; i<chunks->size; i++) {
    pipe->command[i] = parse_command(chunks->data[i]);
  }
  int status = execute_pipeline(pipe);
  free_pipeline(pipe);
  free_str_list(chunks);
  return status;
}

---

There's probably a lot to be said about your parsing code, but with those variable names and way too big scopes, I just skipped right over it. Write helper functions for common tasks. Use ptr[i] instead of *(ptr+i). goto might be ok, but the label name you chose is meaningless.

---

Last quibble:

 printf("Error allocating memory!\n"); /* print an error message */
 int e = 0; /* a counter */

Those comments are not useful. First one is just redundant, second one is yet another indication that your variable names are not good. If it's a counter, give it a name that tells people that it is a counter and what sort of thing it counts!