C - Can't access struct element values from void pointer

Question

Some background:

• I'm a long-time reader but a first-time poster. I have modest experience in Java but very little in C.
• I've created a doubly linked list, where the data in each node is actually a "player" struct.
• I've successfully populated the structs for each node in the list.
• I've done extensive testing on the list itself and it's behaving as expected.
• I want each node in the linked list to have a void pointer, since I plan on using this code for several different types of lists that eventually interact with one another.
• My approach has been to initially store values read from a file in an array, and then read those data into each node's data element. This seems to have been successful.

My problem: When I try to ACCESS the values of the individual elements in the "player" struct, I get nonsense. I'm almost sure this is a pointer issue, as that's been my biggest weakness with C thus far. Pertinent code:

I'm showing the structs just in case I wasn't clear. Note the void pointer in Node, which eventually points to a Player struct:

struct Node;
typedef struct Node {
    void *data;
    struct Node *prev;
    struct Node *next;
} Node;

typedef struct Player {
    char *name;
    char *pos;
    double *num;
} Player ;

I'm including the code I've written where I add the data from the array to the list and print the results. I also show resulting console output.

// char *vals[linesInFile][3] has already been declared.
// List playerList has already been created and initialized; it currently has 0 nodes.
// int i represents the number of nodes in the list.
// getNode and addNode do just what you'd expect them to do.  Both functions have been thoroughly tested.`

Player *player = calloc(1, sizeof(Player));
int n;
for(n = 0; n < i; n++) {
    if(vals[n][0] != NULL) {
        addNode(playerList, n, NULL);
        getNode(playerList, n)->data = &player;
        ((Player *)getNode(playerList, n)->data)->name = vals[n][0];
        ((Player *)getNode(playerList, n)->data)->pos = vals[n][1];
        ((Player *)getNode(playerList, n)->data)->num = vals[n][2];
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->name);
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->pos);
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->num);
        printf("\n");
    }
}

0: Mike Trout
0: CF
0: 4276

1: Bryce Harper
1: LF
1: 4599

2: Jose Fernandez
2: SP
2: 5023

Everything works fine so far. Finally, I'm including the code snippet I've written that attempts to find the values of the player struct from the main function.

// The getSize method has been tested and correctly returns the size of a list.
int i;
for(i = 0; i < getSize(playerList); i++) {
    printf("%d: %s\n", i, ((Player *)getNode(playerList, i)->data)->name);
    // The output from this is garbage.
    char *playerName = ((Player *)getNode(playerList, i)->data)->name;
    printf("%d: %s\n", i, playerName);
    // The output from this is garbage.
}

I'm not positive that all of the code prior this last block is correct, but at very least it seems to be producing desired output. Hopefully this is just an easy pointer problem with the last block of code. Regardless, any help would be greatly appreciated.

Answer 1

This line seems to be wrong:

getNode(playerList, n)->data = &player;

player is already a pointer to a Player, thus, this line is actually setting the new node's data field to a pointer to pointer to Player, and the rest of your code uses it as if it were a pointer to Player - type mismatch, that's your problem.

Instead, that line should be:

getNode(playerList, n)->data = player;

That is, you do not need to apply the address of operator. The rest of the code looks ok.

The code seems to be fine because your printing statements are misleading. You only allocate one node, and keep changing its value over and over. The thing is, for each time you change the fields on that node, you print it, so the output looks correct, but the program is only ever storing one node with one piece of information at a time. You store, print it, overwrite, print, overwrite, print....

To fix this, you need to explicitly allocate a new node on each iteration:

Player *player;
int n;
for(n = 0; n < i; n++) {
    if(vals[n][0] != NULL) {
        player = calloc(1, sizeof(Player));
        addNode(playerList, n, NULL);
        getNode(playerList, n)->data = player;
        ((Player *)getNode(playerList, n)->data)->name = vals[n][0];
        ((Player *)getNode(playerList, n)->data)->pos = vals[n][1];
        ((Player *)getNode(playerList, n)->data)->num = vals[n][2];
    }
}

And then, after populating the list, print it:

for(n = 0; n < i; n++) {
    if(vals[n][0] != NULL) {
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->name);
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->pos);
        printf("%d: %s\n", n, ((Player *)getNode(playerList, n)->data)->num);
        printf("\n");
    }
}

Also, if you don't want to copy the pointers from the array but duplicate its contents, you can use strdup():

        /* ... */
        ((Player *)getNode(playerList, n)->data)->name = strdup(vals[n][0]);
        ((Player *)getNode(playerList, n)->data)->pos = strdup(vals[n][1]);

However, remember that you must now free() each one of name and pos before freeing the node itself. Also, since num is a pointer to double, you can't use strdup on it. Instead, you must manually allocate memory and copy its contents:

        ((Player *)getNode(playerList, n)->data)->num = malloc(sizeof(double));
        *((Player *)getNode(playerList, n)->data)->num = *vals[n][2];

Answer 2

As well as the & issue that Filipe Gonçalves pointed out, you have a couple of other issues.

You only allocate one Player struct and then use it for every item in the list. This will mean they all point to the same data and you will end up printing the last player details i times. You should move the calloc to inside the for n loop.

You are also only copying the pointers for the name, pos, and num values out of vals. This may be fine, but it does mean that vals needs to stay in scope (or not be deallocated) for the lifetime of the player list.