Implementing a Linked List C

Question

I am doing an assignment, and I thought i should use a linked list to store some data. The problem is that the list doesn't retain all nodes.

When adding finishes and I try to see the nodes, it only shows the last node that was added to list.

I will write the relevant part below, I hope someone could point out what the problem is.( I suspect that it must be something related to malloc. the address gets destroyed after the functions finishes its work, but am not sure.

Also I should point out, that I tested, printing data while they were being added, and it did show that they were being added properly to the list).

/**
 * Adds command name and it's hash onto the linked list
 * returns 1, if successful
 * returns 0, if failed
 */

int addToList(struct CMDList *head, char *pathCommand[], char *hash){

int result = 0;

/** If head was pointing to NULL, list empty, add at the beginning */
if(head->path == NULL){

    head->path = pathCommand[0];
    head->command = pathCommand[1];
    head->hash = hash;
    head->next = NULL;
    result = 1;

}else{

    struct CMDList *current = head;

    /** Find tail of the list */
    while(current->next != NULL){

        current = current->next;
    }
    current->next = (struct CMDList *)malloc(sizeof(struct CMDList));
    if(current->next != NULL){

        current->path = pathCommand[0];
        current->command = pathCommand[1];
        current->hash = hash;
        current->next = NULL;
        result = 1;

    }

}

return result;

}

MAIN Program:

int main(int argc, char *argv[]){

/** CODE DELETED */

/** initialize list for storing cmds from config file */
/** cmdList is the head node that i use to traverse the list */
cmdList = (struct CMDList *)malloc(sizeof(struct CMDList));
if(cmdList != NULL){

    cmdList->path = NULL;
    cmdList->command = NULL;
    cmdList->hash = NULL;
    cmdList->next = NULL;
}else{

    printError("Silent Exit: couldn't initialize list to store commands of config file");
    exit(1);
}


/** CODE DELETED **/

/** add new data to the list */
if(!addToList(cmdList,arrayCommand,sha)){

        printError("Silent Exit: couldn't add to list");
        exit(1);
}

}

Standard Warning : Please [do not cast](http://stackoverflow.com/q/605845/2173917) the return value of `malloc()` and family in `C`. — Sourav Ghosh, Mar 30 '15 at 07:24
Citation supporting and providing elaboration upon standard warning: http://c-faq.com/malloc/mallocnocast.html — autistic, Mar 30 '15 at 10:39

score 4 · Accepted Answer · answered Mar 30 '15 at 07:26

4

In this part of your code:

if(current->next != NULL){

    current->path = pathCommand[0];
    current->command = pathCommand[1];
    current->hash = hash;
    current->next = NULL;
    result = 1;

}

You have to use current->next->... instead of current->..., because your new element is in current->next, not in current (in fact, you checked that current->next != NULL).

Because of this error, the first element added is fine, but when you try to add the second, you just allocate its space but then you overwrite the first.

answered Mar 30 '15 at 07:26

lodo

2,314
19
31

The solution did solve the main problem I had. I should have paid more attention. But now there is another problem. The list keeps overwriting all the nodes with same information, so at the end, all nodes contains whatever is added to the last node. I couldn't figure out why it is being overwritten, any ideas? – Neo Mar 31 '15 at 04:22
@Neo Please if you have another problem, post another question. This anwer does solves the problem that you stated in the question, so why did you unaccept it? – lodo Mar 31 '15 at 12:02

orestiss · Answer 2 · 2015-03-30T07:45:48.800

In this part of the code you want to set the variables for the current->next

  if(current->next != NULL){

    current->path = pathCommand[0];
    current->command = pathCommand[1];
    current->hash = hash;
    current->next = NULL;
    result = 1;

}

Another option is, when doing the malloc you set a temp pointer to your new struct, give the values you need and then set current->next = temp;

Also you can skip the while step. You can have a pointer named tail to point at the end of your list.

And do something like

temp=malloc(...)
temp->path = ...
...
tail->next = temp;
tail = temp;

Implementing a Linked List C

2 Answers2