Another List Q - Seg fault on head-tail list

And I am completely stumped as to why i am getting it.. for all intents and purposes, I cannot see a flaw (by eyeballing the code).

Code:

typedef struct list {
	int number;
	struct list *next;
} node;

node *addlist(node *head, node *tail, int number)
{
	node *temp;

	temp = calloc(1, sizeof(node));
	temp->number = number;
    
    tail->next = temp;
    tail = temp;
    
    if ( head == NULL )
    {
        head = tail;
    }
    
    nodecount++;
    
    return tail;

}

void printlist( node *head, node *tail )
{

	node *point;
	point = head;

	while (point)
	{
		printf("Number: %d\n", point->number);
		point = point->next;
	}

}


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

	node *head = NULL;
    node *tail = NULL;

	if ( arguementscheck(argc) != 1 )
	{
		error(1);
	}

	if ( readcheck(argv[1]) != 1 )
	{
		error(2);
	}


	tail = addlist(head, tail, 1);
	tail = addlist(head, tail, 3);
	tail = addlist(head, tail, 5);
	tail = addlist(head, tail, 10);

	printf("Inserted %d nodes into my list.\n", nodecount);

	printlist(head, tail);

	/*
		No errors encountered, all tasks completed.
		Exit the program.
	*/

	stdexit();

}

Well clearly that's not all of your code, because you have a 'nodecount' variable which is apparently global, that isn't shown. How about putting some printf statements scattered around in your code, so you can see how far it gets? Also, you have a bad habbit of not setting your 'next' pointer to NULL. calloc doesn't guarintee pointers end up NULL. It likely ends up that way, but there is at least one implementation, which NULL does not equal all bits zero.


Quzah.

Ok, here is my code again.

It falls over at;

Code:

	/* DEBUG */ printf("tail set to temp.\n");
    tail = temp;

with a seg fault.

Code:

typedef struct list {
	int number;
	struct list *next;
} node;

int nodecount = 0;

node *addlist(node *head, node *tail, int number)
{

	node *temp;
	/* DEBUG */ printf("Created new temp node, calloc. Setting temp->number to number.\n");


	temp = calloc(1, sizeof(node));
	temp->number = number;
    
	/* DEBUG */ printf("tail->next set to temp.\n");
    tail->next = temp;

	/* DEBUG */ printf("tail set to temp.\n");
    tail = temp;
    
    if ( head == NULL )
    {
		/* DEBUG */ printf("head == NULL, so set head = tail\n");
        head = tail;
    }
    
    nodecount++;
    
	/* DEBUG */ printf("Return tail.\n Done.\n\n.");
    return tail;

}



void printlist( node *head, node *tail )
{

	node *point;
	point = head;

	while (point)
	{
		printf("Number: %d\n", point->number);
		point = point->next;
	}

}



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

	node *head = NULL;
	node *tail = calloc(1, sizeof(node));;

	/* DEBUG */ printf("Created nodes head and tail.\n");

	if ( arguementscheck(argc) != 1 )
	{
		error(1);
	}

	if ( readcheck(argv[1]) != 1 )
	{
		error(2);
	}

	/* DEBUG */ printf("Passing head,tail,1 to the addlist function.\n");
	tail = addlist(head, tail, 1);
	tail = addlist(head, tail, 3);
	tail = addlist(head, tail, 5);
	tail = addlist(head, tail, 10);

	printf("Inserted %d nodes into my list.\n", nodecount);
	
	printlist(head, tail);

	/*
		No errors encountered, all tasks completed.
		Exit the program.
	*/

	stdexit();

}

When the first node is created, head and tail are both NULL so when you do:

Code:

    tail->next = temp;

it barfs. tail has to point to some valid memory before you try dereferencing it.

Ok, got that fixed. Now my printlist function isn't printing anything...

That's because head isn't being updated in main(). Everything in C is passed by value. So when you do addlist(head, etc.) you're passing the value that the head pointer contains to addlist(). So head gets updated in the addlist() function because it has it's only local pointer but it remains the same in main().

What you need to do is pass a pointer to head to addlist() like:

Code:

tail = addlist(&head, tail, 1);

And addlist() would then need to be changed to treat head as a pointer to a pointer (node **head). To change head then, you'd need to do *head = tail inside your addlist() function.

Hi;

Thanks again for your help.

I now get the following warning, and still no printing:

--------------------Configuration: Parser - Win32 Debug--------------------
Compiling...
Parser.c
C:\Parser.c(147) : warning C4047: '=' : 'struct list ** ' differs in levels of indirection from 'struct list *'

Parser.obj - 0 error(s), 1 warning(s)

What does your addlist() function look like now?

Originally Posted by quzah

calloc doesn't guarintee pointers end up NULL. It likely ends up that way, but there is at least one implementation, which NULL does not equal all bits zero.
Quzah.

So we must always test pointers against NULL ?

Code:

char  buf[] = "abc";
char  *p;

p = strchr(buf, 'z');

/* This is wrong ? */
if ( ! p )
   printf("Failed to find z\n");
   
/* We must always test it with NULL ? */
if (  p == NULL )
   printf("Failed to find z\n");

Ok got it working Except for one problem now. The printlist() function prints a "0" at the first print.. but I can't see why:

Code:

typedef struct list {
	int number;
	struct list *next;
} node;

node *addlist( node *tail, node *(*head), int number )
{
    node *temp;

    if( (temp = calloc(1, sizeof(node))) == NULL)
    {
        error(3);
    }

    temp->number = number;
    temp->next = NULL;

	if ( *head == NULL )
	{
		*head = tail;
	}

    if ( tail ) 
	{
		tail->next = temp;
		tail = temp;
	}

	nodecount++;

    return tail;
}



void printlist( node *p )
{

	node *point;
	point = p;

	while ( point )
	{
		printf("Number: %d\n", point->number);
		point = point->next;
	}

}



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

	node *head = NULL;
	node *tail = calloc(1, sizeof(node));

	if ( arguementscheck(argc) != 1 )
	{
		error(1);
	}

	if ( readcheck(argv[1]) != 1 )
	{
		error(2);
	}

	tail = addlist(tail, &head, 1);
	tail = addlist(tail, &head, 3);
	tail = addlist(tail, &head, 5);
	tail = addlist(tail, &head, 10);

	printf("Inserted %d nodes into my list.\n", nodecount);
	
	printlist(head);

	/*
		No errors encountered, all tasks completed.
		Exit the program.
	*/

	stdexit();

}

Inserted 4 nodes into my list.
Number: 0
Number: 1
Number: 3
Number: 5
Number: 10
Cleanly exiting program.

You've got a problem here:

Code:

	if ( *head == NULL )
	{
		*head = tail;
	}

    if ( tail ) 
	{
		tail->next = temp;
		tail = temp;
	}

In the first check, tail is still NULL on the first call. You should be setting head to temp, not tail.

In the second check, tail only ever changes if tail is not NULL to begin with. tail is NULL throughout your entire program the way you have it set up. Try this instead:

Code:

if(!*head)
  *head = temp;
if(tail)
  tail->next = temp;
tail = temp;  // This should ALWAYS get set. Not just if tail is not NULL

> So we must always test pointers against NULL ?
No, that's not what he meant.
calloc sets all the bits in the allocated memory to zero, so if you have something like

Code:

typedef struct list {
	int number;
	struct list *next;
} node;

node *p = calloc ( 1, sizeof(node) );  // try and make p->next NULL by filling with zeros
if ( p->next == NULL ) // may fail on some machines.

// You must do this after you call calloc to initialise the data properly
p->next = NULL

This of course makes calloc pretty useless for portable code, since you can only guarantee behaviour for integral types. floats, doubles and pointers cannot be usefully initialised with calloc.
http://c-faq.com/malloc/calloc.html


Testing the return result of allocation functions with either, it doesn't matter.
if ( ! p )
if ( p == NULL )