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!

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; }

---

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

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

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.)

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!

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;

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

Micko.

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

http://oakroadsystems.com/tech/c-predef.htm#ident_

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!