Print linked list: Error - segmentation fault

[deathmetal]

struct node {
    int x;
    struct node *next;
    struct node *previous;
}*start,*tail;

void create(){
    int val=0;
    printf("We are to create new linked list. Enter number\n");
    scanf("%d",&val);

    if(start == NULL){

        start=malloc(sizeof(struct node*)); //create new
        start->next=start->previous=NULL; //make NULL 
        start->x=val;

    }else{
        struct node *temp=start; //create temp node

        while(temp!=NULL){ //this is the problem
            printf("The value is %d\n",temp->x);
            temp=temp->next;
        }
        struct node *new=malloc(sizeof(struct node *));
        new->next=NULL;
        new->previous=temp;
        temp->next=new;
        new->x=val;

    }

}

When I change:
while(temp!=NULL)
to
while(temp->next!=NULL)

I can easily traverse, and print through the list.
If I don't change, I get error as:

Segmentation fault (core dumped)

I' unable to understand why this is occurring?
Please guide.

[GReaper]

Look at what you're doing after the while loop, at line #28.

[laserlight]

This not only defines struct node, but also declares two global pointers:

struct node {
    int x;
    struct node *next;
    struct node *previous;
}*start,*tail;

However, global variables should be avoided, especially when you are a beginner, as they make it more difficult for your to reason about your program and hence to debug/maintain it. Rather, write:

struct node {
    int x;
    struct node *next;
    struct node *previous;
};

Then declare start and tail in main, or within some other function. You could also consider grouping them into a single struct, e.g.,

struct linked_list {
    struct node *start;
    struct node *tail;
};

Then declare a struct linked_list object or pointer in main or within some other function.

Either way, your functions will then be changed to have appropriate parameters, thus allowing you to see more clearly what the functions operate on, and to reuse these functions more easily.

[deathmetal]

Look at what you're doing after the while loop, at line #28.

Hi GR,
Thanks for your reply.
I found one error:

struct node *new=malloc(sizeof(struct node *));

should be changed to

struct node* new=malloc(sizeof(struct node));

Next, at line 28, I made following changes:

        temp=new;
        new->previous=temp;
        new->x=val;

I don't get any crash error. It seems, I am not appending though.

[deathmetal]

Hello laserlight,

This not only defines struct node, but also declares two global pointers:

struct node {
    int x;
    struct node *next;
    struct node *previous;
}*start,*tail;

Yes, to have them global was the purpose.

Then declare start and tail in main, or within some other function. You could also consider grouping them into a single struct, e.g.,

struct linked_list {
    struct node *start;
    struct node *tail;
};

I liked this idea. Thanks much.! http://cboard.cprogramming.com/images/smilies/smile.png

[laserlight]

Allow me to expand on my suggestions in post #3:

/* ... */

struct node {
    int x;
    struct node *next;
    struct node *previous;
};

struct linked_list {
    struct node *start;
    struct node *tail;
};

struct node *append_node(struct linked_list *list, int value) {
    struct node *new_node = malloc(sizeof(*new_node));
    if (new_node) {
        new_node->x = value;
        new_node->next = NULL;

        if (list->start) {
            new_node->previous = list->tail;
            list->tail->next = new_node;
            list->tail = new_node;
        } else {
            new_node->previous = NULL;
            list->start = new_node;
            list->tail = new_node;
        }
    }
    return new_node;
}

void create_list(struct linked_list *list) {
    int val = 0;
    printf("We are to create new linked list. Enter number\n");
    if (scanf("%d", &val) == 1) {
        struct node *temp = append_node(list, val);
        if (temp) {
            printf("The value is %d\n", temp->x);
        } else {
            /* Could not append node, so report or otherwise handle the error */
        }
    } else {
        /* Invalid input, so report or otherwise handle the error */
    }
}

/* ... */

int main(void) {
    struct linked_list list = {NULL};
    create_list(&list);
    /* ... */
    return 0;
}

Doing it this way, you separate the logic of appending a node from the logic of doing interactive I/O and actually building the linked list.

I have taken the liberty of implementing append_node for you as it looks like you did put in effort to think about and implement the logic as per post #1. Notice that:

• Instead of calling malloc in two places like you did, I did it in just one place and did the check for a null pointer before proceeding.
• Instead of setting the new node's value and next pointer in two places like you did, I did it in just one place once it is known that malloc did not return a null pointer.
• Instead of doing a loop to find the tail, I just used the tail pointer.
• Since I separated append_node from create_list, I made it such that create_list (and other functions that might use append_node) can check if append_node succeeded.

Also, while you did correctly fix the malloc call to:

struct node* new=malloc(sizeof(struct node));

Using this instead:

struct node *new_node = malloc(sizeof(*new_node));

avoid having to check that you did use the correct type.

By the way, I think that it is conventional to use head and tail rather than start and tail. If you do want to use start, I would expect start and end. (There's also start and stop as well as start and finish, but those probably would be unconventional for linked lists.)

[deathmetal]

Hello laserlight,

I didn't want to trouble you for the code. I know, it was a cake walk, for you're an expert.

I was/am interested to fix the glitch/but in my code.

• Instead of calling malloc in two places like you did, I did it in just one place and did the check for a null pointer before proceeding.
• Instead of setting the new node's value and next pointer in two places like you did, I did it in just one place once it is known that malloc did not return a null pointer.

Yes, I should have made it more modular, avoided using twice malloc. And keep one function to return node. Also, a check is a must (I'm learning this part).

Also, while you did correctly fix the malloc call to:

struct node* new=malloc(sizeof(struct node));

Using this instead:

struct node *new_node = malloc(sizeof(*new_node));

avoid having to check that you did use the correct type.

>> I didn't quite understand what you imply here, my lack of knowledge is to be held culprit here.

struct node* temp = malloc(sizeof(struct node))
V/s
struct node *temp = malloc(sizeof(*temp))


How do I decide which one to use, when and why?


Thank you again for your constant support, guidance, time and reply.

[laserlight]

I was/am interested to fix the glitch/but in my code.

GReaper pointed out where the error would have manifested, but the mistake was earlier in the code: you terminated the loop when temp is a null pointer, so consequently you cannot dereference temp. To fix this, you should not fix line 28, but rather you should fix the loop such that temp points to the tail node. Hence, your solution to do this works:

while(temp->next!=NULL)

But of course it is more efficient to just use the tail pointer.

>> I didn't quite understand what you imply here, my lack of knowledge is to be held culprit here.

struct node* temp = malloc(sizeof(struct node))
V/s
struct node *temp = malloc(sizeof(*temp))


How do I decide which one to use, when and why?

They are equivalent, but unless temp is a pointer to void, you should use the latter.

The problem with the former is that you could have written, and in fact did write:

temp = malloc(sizeof(struct node*));

and it is easy to miss this, since it looks almost correct and the compiler will not complain. With this:

temp = malloc(sizeof(*temp));

It is clear that the idea is to allocate space for one object of what temp would point to. This would only be a problem if temp is a pointer to void, but at a high enough warning level a compiler is likely to complain.

[deathmetal]

you terminated the loop when temp is a null pointer, so consequently you cannot dereference temp. To fix this, you should not fix line 28, but rather you should fix the loop such that temp points to the tail node.

Yep, I realized the error after a small break.
If the iterator (temp in this case) is NULL, how would it be able to point to anything?

Thank you for the detailed description for different pointer concepts.

Best,
deathmetal