file & linked list run time error

This is a discussion on file & linked list run time error within the C Programming forums, part of the General Programming Boards category; Consider this code Code: #include <stdio.h> #include <stdlib.h> #include <string.h> typedef struct _LIST { int data; struct _LIST *next; }LIST; ...

  1. #1
    Registered User Micko's Avatar
    Join Date
    Nov 2003
    Posts
    712

    file & linked list run time error

    Consider this code

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    typedef struct _LIST
    {
    	int data;
    	struct _LIST *next;
    }LIST;
    
    
    void writefile(LIST *);
    LIST * readfile();
    void Insert(LIST**,LIST *);
    void Free(LIST**);
    void Show(LIST*);
    size_t Length(LIST *);
    
    int main()
    {
    	LIST *head=NULL;
    	LIST node, *dummy;
    	node.data=1;
    	Insert(&head,&node);
    	node.data=2;
    	Insert(&head,&node);
    	node.data=3;
    	Insert(&head,&node);
    	
    	Show(head);
    	
    	writefile(head);
    	
    	//Free(&head);
    	
    	dummy=readfile();
    
    	Show(dummy);
    	
    	return 0;
    }
    
    void writefile(LIST *head)
    {
    	FILE *fp;
    	fp=fopen("List.bin","wb");
    	if(!fp)
    	{
    		printf("\nError opening file!");
    		exit(1);
    	}
    	if((fwrite(head,sizeof(LIST),Length(head),fp))!=Length(head))
    	{
    		printf("Error writing to file!");
    		exit(1);
    	}
    	fclose(fp);
    }
    LIST * readfile()
    {
    	FILE *fp;
    	size_t num;
    	LIST *current;
    	fp=fopen("List.bin","rb");
    	if(!fp)
    	{
    		printf("\nError opening file!");
    		exit(1);
    	}
    	fseek(fp,0L,SEEK_END);
    
    	num=ftell(fp)/sizeof(LIST);
    	
    	rewind(fp);
    
    	current=(LIST *)malloc(sizeof(LIST)*num);
    	if(!current)
    	{
    		printf("\nMemory allocation error!");
    		exit(1);
    	}
    	
    	if((fread(current,sizeof(LIST),num,fp))!=num)
    	{
    		printf("Error reading file!");
    		exit(1);
    	}
    
    	fclose(fp);
    	return current;
    
    }
    void Insert(LIST **head,LIST *node)
    {
    	LIST *current=(LIST *)malloc(sizeof(LIST));
    	LIST *dummy;
    	if(!current)
    	{
    		printf("Memory allocation error!");
    		exit(1);
    	}
    	memcpy(current,node,sizeof(LIST));
    	current->next=NULL;
    	if(!*head)
    	{
    		*head=current;
    		return;
    	}
    	dummy=*head;
    	*head=current;
    	current->next=dummy;
    }
    void Free(LIST **head)
    {
    	LIST *dummy;
    	while(*head !=NULL)
    	{
    		dummy=*head;
    		*head=dummy->next;
    		free (dummy);
    /* mybe I have memory leak here */
    	}
    	
    }
    size_t Length(LIST *head)
    {
    	size_t len=0;
    	LIST *dummy;
    	for(dummy=head;dummy!=NULL;dummy=dummy->next)
    		len++;
    	return len;
    }
    void Show(LIST *head)
    {
    	LIST *dummy;
    	if(!head)
    	{
    		printf("\nList is empty!");
    		return;
    	}
    	for(dummy=head;dummy !=NULL;dummy=dummy->next)
    		printf("%d ",dummy->data); 
    }
    Everything seems to works fine but my program has strange bug that I can't figure out.
    When I uncomment line //Free(&head); my program crashes in run time.
    If I uncomment this line Free(&head) and put in comment line: Show(dummy) then again everything works
    I can't see connection between function free and open files.
    My idea is I create linked list, write in file, close file, delete list, open file read from it and then close again
    maybe problem is using malloc in line:current=(LIST *)malloc(sizeof(LIST)*num);
    but it works if Free is not called.
    Can you debug this and tell what is causing problems.
    I don't see how Free list can affect this, because I have list written in the file!
    Thank you very much for your help!

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,499
    Well your write() and read() to/from a file is WAY off.

    > fwrite(head,sizeof(LIST),Length(head),fp
    malloc does not guarantee that memory is allocated in contiguous increasing addresses.

    You need to write out each node separately
    Code:
    while ( node != NULL ) { write(node); node = node->next; }
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User
    Join Date
    Mar 2004
    Posts
    536
    Check your file List.bin. Your program crashes in the Show(dummy) after the readfile().

    Your writefile() routine (in your test case in this program) writes 24 bytes starting from the current head address -- the last thing malloc()ed.

    I think you have to traverse the list, writing the list elements one at a time (and read them back in the same way to reconstruct the list.)

    Debugging suggestion:

    Sprinkle a few printf()s throughout the program showing pointer values (using %p).

    Dave

  4. #4
    Registered User Micko's Avatar
    Join Date
    Nov 2003
    Posts
    712
    Thanks guys ,but interesting when remove function Free then everything works fine. I don't know how this affect because I open file, write and close then I deallocate list. You are right regarding writing but this specific case is interesting I don't know why

  5. #5
    Registered User
    Join Date
    Mar 2004
    Posts
    536
    Because in your program you are writing out the pointers that malloc() gave you when you Insert()-ed your elements. Then
    you deallocate the storage (in Free()), and read back the pointers to the (now-non-existent) storage!



    A suggestion:
    Don't read back the pointers and expect to be able to use the memory! (In your readfile(), read the data item and then use Insert to create your new list.)

  6. #6
    Registered User Micko's Avatar
    Join Date
    Nov 2003
    Posts
    712
    I finally did it after a lot of hard work. I've lost all afternoon for this.
    This is working but I'm not satisfied. I know it could be better and easier and perhaps more elegant.
    Plese check this and suggest me what to fix and optimize

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    typedef struct _LIST
    {
    	int data;
    	struct _LIST *next;
    }LIST;
    
    
    void writefile(LIST *);
    void readfile(LIST **);
    void Insert(LIST**,LIST *);
    void Free(LIST**);
    void Show(LIST*);
    size_t Length(LIST *);
    void InsertEnd(LIST **,LIST *);
    int main()
    {
    	LIST *head=NULL,*dummy=NULL;
    	LIST node;
    	clrscr();
    	node.data=1;
    	Insert(&head,&node);
    	node.data=2;
    	Insert(&head,&node);
    	node.data=3;
    	Insert(&head,&node);
    	node.data=4;
    	Insert(&head,&node);
    	writefile(head);
    
    	/*Free(&head);*/
    
    	readfile(&dummy);
    
    	Show(dummy);
    
    	return 0;
    }
    
    void writefile(LIST *head)
    {
    	FILE *fp;
    	LIST *dummy;
    	fp=fopen("List.bin","wb");
    	if(!fp)
    	{
    		printf("\nError opening file!");
    		exit(1);
    	}
    	for(dummy=head;dummy!=NULL;dummy=dummy->next)
    		if((fwrite(dummy,sizeof(LIST),1,fp))!=1)
    		{
    			printf("\nError writing file!");
    			exit(1);
    		}
    
    	fclose(fp);
    }
    void readfile(LIST **head)
    {
    	LIST *current;
    	FILE *fp;
    	fp=fopen("List.bin","rb");
    	current=(LIST *)malloc(sizeof(LIST));
    	while((fread(current,sizeof(LIST),1,fp))==1)
    	{
    
    		InsertEnd(head,current);
    	}
    
    
    }
    void Insert(LIST **head,LIST *node)
    {
    	LIST *current=(LIST *)malloc(sizeof(LIST));
    	LIST *dummy;
    	if(!current)
    	{
    		printf("Memory allocation error!");
    		exit(1);
    	}
    	memcpy(current,node,sizeof(LIST));
    	current->next=NULL;
    	if(!*head)
    	{
    		*head=current;
    		return;
    	}
    	dummy=*head;
    	*head=current;
    	current->next=dummy;
    }
    
    void InsertEnd(LIST **head,LIST *node)
    {
    	LIST*current=*head;
    	LIST *newnode=(LIST *)malloc(sizeof(LIST));
    
    	memcpy(newnode,node,sizeof(LIST));
    	newnode->next=NULL;
    
    	if(*head==NULL)
    	{
    		*head=newnode;
    		return;
    	}
    	while(current->next !=NULL)
    		current=current->next;
    	current->next=newnode;
    
    }
    
    void Free(LIST **head)
    {
    	LIST *dummy;
    	while((*head)->next !=NULL)
    	{
    		dummy=*head;
    		*head=dummy->next;
    		free (dummy);
    	}
    	free(*head);
    	*head=NULL;
    }
    size_t Length(LIST *head)
    {
    	size_t len=0;
    	LIST *dummy;
    	for(dummy=head;dummy!=NULL;dummy=dummy->next)
    		len++;
    	return len;
    }
    void Show(LIST *head)
    {
    	LIST *dummy;
    	if(!head)
    	{
    		printf("List is empty!");
    		return;
    	}
    	for(dummy=head;dummy !=NULL;dummy=dummy->next){
    		printf("%d ",dummy->data);
    	}
    }
    ;

    I had terrible problems reading file so I add new function InsertEnd and change prototype of readfile().
    Any comments are welcome!

  7. #7
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    Whe starting out with malloc/free, its a good idea to keep track of how many times you call each function. That way you can spot memory leaks easily.

    If you want to do this, the easiest way, albeit a quick bodge, is to create 2 globals variables (say called MallocCalls and FreeCalls), both being ints, and increment their value every time you call the respective function. At the end of the program, print both values

    current = malloc(sizeof(LIST));
    MallocCalls++;
    ...
    printf ("FreeCalls %d, MallocCalls %d\n", FreeCalls, MallocCalls);

    The numbers should match if you've done your job right. If not, you need to go back and debug!

    Your show() function (and some others) can be simplified, as you don't need a local dummy variable. Some people like to use them anyway, so this is really up to you. Example:
    Code:
    void Show(LIST *head)
    {
      if (head == NULL)
      {
        printf("List is empty!\n");
        return;
      }
    
      while (head)
      {
        printf("%d ", head->data);
        head = head->next;
      }
    }
    >>current = (LIST *) malloc(sizeof(LIST));
    The cast isn't needed, read here:
    http://faq.cprogramming.com/cgi-bin/...&id=1043284351

    >>fp = fopen("List.bin", "rb");
    Check that fp isn't NULL before you use it.

    >>fclose(fp);
    You only have one of these in your program, but you open the file twice? The readfile() function is missing one.

    >>typedef struct _LIST
    _LIST is a reserved name, avoid using a leading underscore in your variable names. My personal preference would be something like this:
    Code:
    typedef struct list
    {
      int           data;
      struct list  *next;
    } list_t;
    >>memcpy(current, node, sizeof(LIST));
    As you're dealing with structs, you can use direct assignment if you like, meaning replace the above with:
    >>*current = *node;
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  8. #8
    Registered User
    Join Date
    Mar 2004
    Posts
    536
    My main question (style rather than substance): Why do you write the ->next pointers to the output file? (You copy the entire LIST entry.) When you read the data back in, you only need (and, in fact only use) the data part, since your InsertEnd() correctly uses newly malloc()ed memory.

    Why not just write the data part?

    Here's the sequence that I see you have implemented:

    Build list by inserting new items into head of the list. (Was this the assignment, or did it matter?)

    Write to file (so newest item is the first thing in the file).

    Read from file, but put as each item is read, put it at the end
    of the list. (So this reconstructed list now has same sequence as the original.)

    Is this what you want? If so: GOOD JOB! Take a break.


    P.S. Have you tried it lately with the Free() in place? Seems to work OK now, I think.


    Dave

  9. #9
    c99
    c99 is offline
    Registered User
    Join Date
    Feb 2004
    Posts
    79
    Micko.

    Off topic but worth mentioning, you shouldn't begin identifiers with _

    http://oakroadsystems.com/tech/c-predef.htm#ident_
    R.I.P C89

  10. #10
    Registered User Micko's Avatar
    Join Date
    Nov 2003
    Posts
    712
    I accepted most of yours advices and this is finally version:
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    typedef struct list
    {
    	int data;
    	struct list *next;
    }LIST;
    
    int MallocCalls=0;
    int FreeCalls=0;
    
    void writefile(LIST *);
    LIST * readfile();
    void Insert(LIST**,LIST *);
    void InsertEnd(LIST **,LIST *);
    void Free(LIST**);
    void Show(LIST*);
    size_t Length(LIST *);
    
    int main()
    {
    	LIST *head=NULL,*dummy=NULL;
    	LIST node;
    	
    	node.data=1;
    	Insert(&head,&node);
    	node.data=2;
    	Insert(&head,&node);
    	node.data=3;
    	Insert(&head,&node);
    	node.data=4;
    	Insert(&head,&node);
    	writefile(head);
    	Free(&head);
    	printf("\nAfter Frre() length of list is %d\n",Length(head));
    	dummy=readfile();
    	printf("\nAfter reading file length of list is %d\n",Length(dummy));
    	Show(dummy);
    	Free(&dummy);
    	printf ("\nFreeCalls %d, MallocCalls %d\n", FreeCalls, MallocCalls);
    	return 0;
    }
    
    void writefile(LIST *head)
    {
    	FILE *fp;
    	LIST *dummy;
    	fp=fopen("List.bin","wb");
    	if(!fp)
    	{
    		printf("\nError opening file!");
    		exit(1);
    	}
    	for(dummy=head;dummy!=NULL;dummy=dummy->next)
    		if((fwrite(dummy,sizeof(LIST),1,fp))!=1)
    		{
    			printf("\nError writing file!");
    			exit(1);
    		}
    
    	fclose(fp);
    }
    
    
    LIST * readfile()
    {
    	LIST *current,*head=NULL;
    	FILE *fp;
    	fp=fopen("List.bin","rb");
    	if(!fp)
    	{
    		printf("\nError opening file!");
    		exit(1);
    	}
    	current=(LIST *)malloc(sizeof(LIST));
    	if(!current)
    	{
    		printf("\nMemory allocation error!");
    		exit(1);
    	}
    	MallocCalls++;
    	while((fread(current,sizeof(LIST),1,fp))==1)
    	{
    
    		InsertEnd(&head,current);
    	}
    
    	free(current);
    	FreeCalls++;
    
    	return head;
    }
    
    
    void Insert(LIST **head,LIST *node)
    {
    	LIST *current=malloc(sizeof(LIST));
    	LIST *dummy;
    	MallocCalls++;
    	if(!current)
    	{
    		printf("Memory allocation error!");
    		exit(1);
    	}
    	memcpy(current,node,sizeof(LIST));
    	current->next=NULL;
    	if(!*head)
    	{
    		*head=current;
    		return;
    	}
    	dummy=*head;
    	*head=current;
    	current->next=dummy;
    }
    
    void InsertEnd(LIST **head,LIST *node)
    {
    	LIST*current=*head;
    	LIST *newnode=malloc(sizeof(LIST));
    	MallocCalls++;
    	memcpy(newnode,node,sizeof(LIST));
    	newnode->next=NULL;
    
    	if(!*head)
    	{
    		*head=newnode;
    		return;
    	}
    	while(current->next !=NULL)
    		current=current->next;
    	current->next=newnode;
    
    }
    
    
    void Free(LIST**head)
    {
    	LIST *dummy;
    	while((*head)->next !=NULL)
    	{
    		dummy=*head;
    		*head=dummy->next;
    		free (dummy);
    		FreeCalls++;
    	}
    	free(*head);
    	FreeCalls++;
    	*head=NULL;
    }
    size_t Length(LIST *head)
    {
    	size_t len=0;
    	LIST *dummy;
    	for(dummy=head;dummy!=NULL;dummy=dummy->next)
    		len++;
    	return len;
    }
    void Show(LIST *head)
    {
      if (!head)
      {
        printf("List is empty!\n");
        return;
      }
    
      while (head)
      {
        printf("%d ", head->data);
        head = head->next;
      }
    }
    I'm not satisfied with only one thing: function InsertEnd which I use for restoring list from file
    It would be good to write in file in reverse order and then use Insert function that already exists
    but then I'll probably have to complicate for loops in writefile and taht job is error prone, so I suppose
    this is also good solution.
    With global variables for counting malloc and free I discovered memory leak in fuction readfile and added free(current)
    Thanks everyone for help!

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Avoiding Global variables
    By csonx_p in forum Windows Programming
    Replies: 32
    Last Post: 05-19-2008, 12:17 AM
  2. Using VC Toolkit 2003
    By Noobwaker in forum Windows Programming
    Replies: 8
    Last Post: 03-13-2006, 06:33 AM
  3. Problem with Visual C++ Object-Oriented Programming Book.
    By GameGenie in forum C++ Programming
    Replies: 9
    Last Post: 08-29-2005, 11:21 PM
  4. Replies: 3
    Last Post: 03-04-2005, 01:46 PM
  5. Learning OpenGL
    By HQSneaker in forum C++ Programming
    Replies: 7
    Last Post: 08-06-2004, 08:57 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21