Thread: Segmentation Fault in Doubly Linked List Program.. Can not figure out why

I think you're doing way too much work.

If your aim is to prepend the new node, then:

Code:

new node's prev node = null
if list is empty
    new node's next node = null
    tail = new node
else
    new node's next node = head
    head's prev node = new node
head = new node

If your aim is to append the new node, then:

Code:

new node's next node = null
if list is empty
    new node's prev node = null
    head = new node
else
    new node's prev node = tail
    tail's next node = new node
tail = new node

Either way, you definitely don't need a loop because you're keeping track of both the head and the tail. You would only need a loop if say, you were trying to insert in the middle so as to maintain ordering by movie title or something like that.

I would also be concerned about this line:

Code:

newNode->data=m;

This assumes that the caller is correctly managing the memory for the struct MovieType object. If that is not done correctly, you could get a segmentation fault.

EDIT:
Oh, I didn't notice that you need to set the previous node pointer too, so I've updated my pseudocode.

Originally Posted by Elenasnow

Would I need to free m every time after I add each movie to the list? Do you think that's the only place the segmentation error could be coming from?

No, you don't want to do that at all. You want the memory to remain allocated until you are done with it, which is probably when you have no more use for that node or for the entire linked list. It is a premature deallocation of the memory for the struct MovieType object that can result in a segmentation fault.

Note that if this is literally what you wrote, then it is probably wrong and should result in at least a compile warning:

Code:

*m = (struct MovieType *) malloc(sizeof(struct MovieType));

You probably wanted to write:

Code:

m = (struct MovieType *) malloc(sizeof(struct MovieType));

But I would recommend:

Code:

m = malloc(sizeof(*m));

EDIT:
I think it would be helpful to explain how do you intend to insert the new node. Are you prepending, appending, or inserting in another manner (e.g., you do have an enum SortType s parameter)? It looks like you're correctly prepending when the list is either empty or only has one element, but I don't understand what the loop is for. The loop condition makes it sound like you want to append, but that's inconsistent with what you do when the list only has one element, and then the return statement in the loop body means the loop really only runs for at most one iteration.

Originally Posted by laserlight

I think it would be helpful to explain how do you intend to insert the new node. Are you prepending, appending, or inserting in another manner (e.g., you do have an enum SortType s parameter)? It looks like you're correctly prepending when the list is either empty or only has one element, but I don't understand what the loop is for. The loop condition makes it sound like you want to append, but that's inconsistent with what you do when the list only has one element, and then the return statement in the loop body means the loop really only runs for at most one iteration.

Mostly right now I'm just trying to append the node to the list, eventually I want to sort it based on the list contents but right now I'm just focusing on getting rid of the segmentation error, I made the recommended changes but still can't manage to figure out what I'm doing to cause it..

What is your current code?

Here's what I suggest:

Code:

void addMovieToList(enum SortType s, struct ListType *list, struct MovieType *m) {
    NodeType *newNode = malloc(sizeof(*newNode));
    if (newNode == NULL) {
        printf("mem allocation error");
        exit(0);
    }

    newNode->data = m;
    newNode->data->id = list->nextid++;

    // prepend the new node to the linked list:
    newNode->prev = NULL;
    if (list->head == NULL)
    {
        newNode->next = NULL;
        list->tail = newNode;
    }
    else
    {
        newNode->next = list->head;
        list->head->prev = newNode;
    }
    list->head = newNode;
}

What I did:

• Removed all the other variables that you don't need (other than the s parameter, which you indicated that you will need later)
• Changed the malloc to the version that I recommended earlier, but it is otherwise equivalent.
• Post-incremented the nextid, otherwise it wouldn't be a nextid the next time
• Set the new node's previous node pointer to be a null pointer since we're prepending
• Replaced currNode with list->head
• Removed the early return statements that you don't need

Notice that I also added a comment summarising to the reader that the code prepends the new node to the linked list. You wrote that "Mostly right now I'm just trying to append the node to the list", but the code that you wrote (presumably following my pseudocode) prepends instead of appending.

It doesn't really matter right now, but you might want to return something to indicate the function failed instead of aborting when malloc returns a null pointer. If you really do want to abort, it would be more typical to write to stderr and exit with a non-zero status code, e.g.,

Code:

fprintf(stderr, "mem allocation error");
exit(EXIT_FAILURE);

This way, someone running your program could pipe stderr to an error log separate from normal output, and a shell script could check the status code to determine if your program "succeeded" or "failed".

EDIT:
Also, you made a mistake here:

Code:

void initMovie(char *t, int y, struct MovieType **m)
{
    *m = (struct MovieType *) malloc(sizeof(struct MovieType));
    if (m == NULL) {
        printf("Memory allocation error\n");
        exit(0);
    }
    strcpy((*m)->movie_title, t);
    (*m)->year = y;
   // printf("%s, %d \n", (*m)->movie_title, (*m)->year);
}

Basically, your m == NULL check is wrong because you want to check the return value of malloc instead. You should check for *m == NULL. But an easier way is to do something like this:

Code:

void initMovie(char *t, int y, struct MovieType **m)
{
    struct MovieType *p = malloc(sizeof(*p));
    if (p == NULL) {
        printf("Memory allocation error\n");
        exit(0);
    }
    strcpy(p->movie_title, t);
    p->year = y;
    *m = p;
}

That is, you introduce a local variable so that you can avoid the hassle of dereferencing the pointer to a pointer again and again.

As a matter of good style, your parameter names should be more descriptive. A possible exception to this is when your type names already describe the variable very well (e.g., we can easily see that m refers to a movie, though it is probably still no harm to name it movie), if not it is unclear what t and y are for without reading the body of the function. Another improvement that you could make is to change the type of the title parameter to const char*. This indicates that it is safe to pass a string literal as the title because initMovie will not attempt to modify the title, and indeed your implementation of initMovie only copies the given title.

No, I haven't compiled and run your code.

Looking around, you do have a problem with list_init (which really should be named initList for consistency). Notice that you used a pointer to a pointer for initMovie, but here you only have a pointer to the list itself.