Creating a struct

Question

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

chux · Accepted Answer · 2016-05-05 21:19:36Z

Mixed use of array indexing with unsigned and int. size_t should be used.
Rather than *pString1 = "\0";, *pString1 = ""; is certainly sufficient.
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) {
```
Over-size allocation matrix[row] = malloc(rows * sizeof(char *));. matrix[row] is an allocation to an array of char, not an array of char *.
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);
```

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

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, '|');

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) {

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?

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. — Programmer 400, May 6 at 7:13
Can alloc_argv and alloc_matrix be the same since an array is a matrix? — Programmer 400, May 6 at 7:23
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? — Programmer 400, May 6 at 7:25
@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 *. — chux, May 6 at 13:03

asked	1 month ago
viewed	42 times
active	1 month ago

current community

your communities

more stack exchange communities

Creating a struct

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged c data-structures shell pointers posix or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Creating a struct

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c data-structures shell pointers posix or ask your own question.

Related

Hot Network Questions