Can someone help me understand how to allocate properly in c?

Question

I don't think I properly understand how to allocate memory for what I want to do.

I would like my program to store arguments from the command line into an array of stucts called Command which has char **args in it. for example if I run

./test.c echo hello : ls -l

I want it to store it as this

commands[0].args[0]= echo
commands[0].args[1]= hello
commands[1].args[0]= ls
commands[1].args[1]= -l

But instead my code is storing it in this way

commands[0].args[0]= echo
commands[0].args[1]= hello
commands[0].args[2]= ls
commands[0].args[3]= -l
commands[1].args[0]= ls
commands[1].args[1]= -l

Could someone help me understand why it is storing ls -l in 2 places? Here is my code:

#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>

typedef struct test {
   char **args;
} Command;


int main(int argc, char *argv[])
{
   int i, j, k;
   Command *commands;

   j = k = 0;
   commands = (Command *)malloc(argc * sizeof(Command));

   for (i = 1; i < argc; i++)
   {
      if (strcmp(argv[i], ":") == 0)
      {
         j++;
         k = 0;
      }
      else {
         commands[j].args = (char **)realloc(commands[j].args, (k+1) * sizeof(char*));
         commands[j].args[k++] = argv[i];
      }
   }

   for (i = 0; i <= j; i++)
   {
      for (k = 0; k < 5; k++)
      {
         printf("commands[%d].args[%d]= %s\n", i, k, commands[i].args[k]);
      }
   }
   return EXIT_SUCCESS;
}

Answer 1

Each of your Command structs only have one arg

Perhaps you should consider

typedef struct test {
   char **args[5];
} Command;

and then design a better data structure, like a list of lists.

Answer 2

Your data storage structure has no way of telling how many strings in commands[j] are valid. So I think it's putting two pointers each in commands[0] and commands[1] just like you expect. But then your print loop looks at commands[0].args[k] for k all the way up to 4, even though it's only valid to look at the first two. When you get up to looking at commands[0].args[2], the result is undefined. (Showing memory from somewhere else in your program, crashing, and catching fire are just a few of the things a program is allowed to do if you use undefined behavior.)

To figure out how many arguments are in each command, you could add a counter member to your struct test. Or maybe allocate one more pointer than there are arguments, and put a NULL after the last argument.

Answer 3

Perhaps you should store the length of args in the struct?

typedef struct test {
    char ** args;
    unsigned length;
} Command;

Also, maybe you should consider using some of the built in functionality of the C string library. For example, strtok splits a string using the delimiters you give it.

Answer 4

Here is how I would allocate the memory:

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

typedef struct cmd_s {
    int num;
    char **args;
} cmd_t;

void print_cmds(cmd_t *c, int num) {
    int i, j;
    for (i=0;i<=num;i++) {
        for (j=0;j<c[i].num;j++)
            printf("cmds[%d][%d] = %s\n", i, j,c[i].args[j]);
    }
}

int main(int argc, char *argv[]) {
    int i, j = 0, k = 0;
    cmd_t *cmds;

    cmds = (cmd_t *)malloc(sizeof(cmd_t));
    cmds[0].args = NULL;
    cmds[0].num = 0;

    for (i=1;i<argc;i++) {
        if (strcmp(argv[i], ":") == 0) {
            cmds = (cmd_t *)realloc(cmds, (sizeof(cmd_t) * ++j) + 1);
            cmds[j].args = NULL;
            cmds[j].num = 0;
            continue;
        }
        cmds[j].args = (char **)realloc(cmds[j].args, sizeof(char *) * ++cmds[j].num);
        cmds[j].args[cmds[j].num-1] = (char *)malloc(50);
        strcpy(cmds[j].args[cmds[j].num-1], argv[i]);
    }

    print_cmds(cmds, j);

    for (i=0;i<=j;i++) {
        for(k=0;k<cmds[i].num;k++)
            free(cmds[i].args[k]);
        free(cmds[i].args);
    }

    free(cmds);
    return 0;
}