-4
\$\begingroup\$

Mat told me to create a struct

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

So i did.

struct str_list {
    char *const *name;
    int argc;
    char *const *argv;
};

Is it ok? The goal is 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;
}

The code actually looks like this

static int runCmd(const char *cmd) {
    int freeme_len = 0;
    char *pString1[z];
    char *pString[z];
    *pString1 = "\0";
    *pString = "\0";
    bool quote = false;
    int argc = 1;
    int n = 0;
    int status = 0;
    int w = 0;
    int i2 = 0;

    struct command shellcommand[BUFFER_SIZE];

    char **ptr1;
    char **argv = alloc_argv(BUFFER_SIZE);
    char **new_argv = alloc_argv(BUFFER_SIZE);
    char ***matrix = alloc_matrix(BUFFER_SIZE, BUFFER_SIZE);
    char ***pString4 = alloc_matrix(BUFFER_SIZE, BUFFER_SIZE);

    char *cmdtmp;
    char *pString3[64];
    cmdtmp = strdup(cmd);
    ptr1 = str_split(pString3, cmdtmp, '|');
    for (int i = 0; ptr1[i]; i++) { /* loop for each pipeline */
        n++;                 /* save number of pipelines */
        char *string1[z];
        int e = 0;
        *pString = "\0";
        *string1 = strdup(ptr1[i]);
        if ((*string1[0] != '\0') &&
            !isspace(*string1[0])) {
            w = parse_command(w, new_argv, string1, pString, &i2, n, quote, i, matrix, pString1,
                              e, &argc, argv, pString4, &freeme_len);;
        }
        free(*string1);
    }
    for (int i = 0; i < n; i++) {
        shellcommand[i].argv = matrix[i];
    }
    fflush(NULL);
    /* refactor to a function */
    pid_t pid;
    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, shellcommand);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */
    while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));
    if (pid < 0) {
        fprintf(stderr, "Error from waitpid: %s", strerror(errno));
        free(cmdtmp);
        if (ptr1) {
            for (int i = 0; ptr1[i]; i++) {
                free(ptr1[i]);
            }
            printf("\n");
            free(ptr1);
        }
        return -1;
    }
    if (WIFSIGNALED(status)) {
        fprintf(stderr, "pid %ld: killed by signal %d\n",
                (long) pid, WTERMSIG(status));
        free(cmdtmp);
        if (ptr1) {
            for (int i = 0; ptr1[i]; i++) {
                free(ptr1[i]);
            }
            printf("\n");
            free(ptr1);
        }
        return -1;
    }
    free(cmdtmp);
    if (ptr1) {
        for (int i = 0; ptr1[i]; i++) {
            free(ptr1[i]);
        }
        free(ptr1);
    }

    free_matrix(matrix, BUFFER_SIZE, BUFFER_SIZE);
    free_matrix(pString4, BUFFER_SIZE, BUFFER_SIZE);
    free_argv(argv, BUFFER_SIZE);
    free_argv(new_argv, BUFFER_SIZE);

    return WEXITSTATUS(status);
}

My allocation and deallocation functions look like the following but I didn't do it yet for a `struct only for arrays and matrices but I think that I can have an array of structs with one struct and use these functions for the struct.

*/
/* returns an array of arrays of char*, all of which NULL */
char ***alloc_matrix(unsigned rows, unsigned columns) {
    char ***matrix = malloc(rows * sizeof(char **));
    if (!matrix) abort();

    for (unsigned row = 0; row < rows; row++) {
        matrix[row] = malloc(columns * sizeof(char *));
        if (!matrix[row]) abort();
        for (unsigned column = 0; column < columns; column++) {
            matrix[row][column] = NULL;
        }
    }
    return matrix;
}

/* returns an array of char*, all of which NULL */
char **alloc_argv(unsigned rows) {
    char **matrix = malloc(rows * sizeof(char *));
    if (!matrix) abort();

    for (unsigned row = 0; row < rows; row++) {
        matrix[row] = malloc(rows * sizeof(char *));
        if (!matrix[row]) abort();

    }
    return matrix;
}

/* deallocates an array of arrays of char*, calling free() on each */
void free_argv(char **argv, unsigned rows) {
    for (unsigned row = 0; row < rows; row++) {

        free(argv[row]);
    }
    free(argv);
}

/* deallocates an array of arrays of char*, calling free() on each */
void free_matrix(char ***matrix, unsigned rows, unsigned columns) {
    for (unsigned row = 0; row < rows; row++) {
        for (unsigned column = 0; column < columns; column++) {
            //    printf("column %d row %d\n", column, row);
            free(matrix[row][column]);
        }
        free(matrix[row]);
    }
    free(matrix);
}

My forking and exec looks like these

/* Helper function that spawns processes */
int spawn_proc(int in, int out, struct command *cmd) {
    pid_t pid;
    fflush(NULL);
    pid = fork();
    if (pid == 0) {
        if (in != 0) {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->argv[0]);
            close(in);
        }
        if (out != 1) {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->argv[0]);
            close(out);
        }
        /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd->argv[0]);*/
        fprintf(stderr, "[%d]\n", (int) getpid());
        execvp(cmd->argv[0], cmd->argv);
        err_syserr("failed to execute %s: ", cmd->argv[0]);
    }
    else if (pid < 0) {
        err_syserr("fork failed: ");
    } else {
        /* printf("** we are the parent ***"); */
    }
    return pid;
}

/* Helper function that forks pipes */
void fork_pipes(int n, struct command *cmd) {
    int i;
    int in = 0;
    int fd[2];

    for (i = 0; i < n - 1; ++i) {

        if (pipe(fd) == -1) {
            err_syserr("Failed creating pipe");
        }

        spawn_proc(in, fd[1], cmd + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0) {
        err_syserr("dup2() failed on stdin for %s: ", cmd[i].argv[0]);
    }
    /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd[i].argv[0]);*/
    fprintf(stderr, "\n");
    execvp(cmd[i].argv[0], cmd[i].argv);
    err_syserr("failed to execute %s: ", cmd[i].argv[0]);
}
\$\endgroup\$
5
\$\begingroup\$
  1. Mixed use of array indexing with unsigned and int. size_t should be used.

  2. Rather than *pString1 = "\0";, *pString1 = ""; is certainly sufficient.

  3. Miss-guided comment. There is no NULL-ness in the array - just some uninitialized values in matrix[row][]. Could use calloc() instead.

    /* returns an array of char*, all of which NULL */
    char **alloc_argv(unsigned rows) {
    
  4. Over-size allocation matrix[row] = malloc(rows * sizeof(char *));. matrix[row] is an allocation to an array of char, not an array of char *.

  5. Suggested using sizeof *pointer * N rather than N * sizeof type in malloc(). Less chance to code incorrectly as OP did originally.

    // matrix[row] = malloc(rows * sizeof(char *));
    matrix[row] = malloc(sizeof *(matrix[row]) * rows);
    
  6. When de-allocating, especially in debug mode, consider setting the pointer to NULL. This helps detects invalid subsequent usage.

    /* deallocates an array of arrays of char*, calling free() on each */
    void free_argv(char **argv, unsigned rows) {
      for (unsigned row = 0; row < rows; row++) {
        free(argv[row]);
        argv[row] = NULL; // add
      }
      free(argv);
    }
    
  7. Declare when code can be given a value: RAII.

    // char **ptr1;
    // ... Many lines later
    // ptr1 = str_split(pString3, cmdtmp, '|');
    
    char **ptr1 = str_split(pString3, cmdtmp, '|');
    
  8. if (ptr1) { serves no purpose - it is too late as ptr1[i] occurred beforehand. Maybe add if (ptr1) { right after

    ptr1 = str_split(pString3, cmdtmp, '|');
    for (int i = 0; ptr1[i]; i++) { /* loop for each pipeline */
    ... dozens of lines later
    if (ptr1) {
    
  9. IMO free_like() functions should handle free-ing NULL just like free(NULL) works.

    void free_argv(char **argv, unsigned rows) {
      if (argv) {
        ...
      }
    

In general, the code functionality was not clear like what is z in char *pString1[z]; and why is alloc_argv() allocating rows*rows?

\$\endgroup\$
  • \$\begingroup\$ You will get a bounty a.s.a. p. because I like this answer. z was my quickfix for a constant that I changed to BUFFER_SIZE i e array length. My programming was focused on getting correct output with bad code and then fixing the code because users usually don't care about variable names and usability is king. \$\endgroup\$ – Niklas May 6 '16 at 7:13
  • \$\begingroup\$ Can alloc_argv and alloc_matrix be the same since an array is a matrix? \$\endgroup\$ – Niklas May 6 '16 at 7:23
  • \$\begingroup\$ If I use calloc instead of malloc, will the code be ` matrix[row] = calloc(columns , sizeof(char *));` ? The matrix code should be able to handle the array data, do you agree? \$\endgroup\$ – Niklas May 6 '16 at 7:25
  • 2
    \$\begingroup\$ @Programmer 400 matrix[row] = calloc(columns , sizeof(char *)); is a problem as alloc_argv(unsigned rows) does not pass in columns and matrix[row] points to a char, not a char *. \$\endgroup\$ – chux - Reinstate Monica May 6 '16 at 13:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.