Reputation: 557
So, here is the thing. I have this program which stores different authors in a linked list, where each author has his own records and each record holds his texts, organized in a linked list. The data are read from a file. Here is the code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
struct AuthorRecord
{
char textTitle[100];
int NumberOfWords;
long Download;
struct AuthorRecord *next;
};
typedef struct AuthorRecord *AuthorRecordType;
typedef struct
{
char firstName[30];
char lastName[30];
int idNumber;
int textNum;
AuthorRecordType text;
} AuthorType;
struct MemberNodeStruct
{
AuthorType *anAuthor;
struct MemberNodeStruct *next;
};
typedef struct MemberNodeStruct *MemberNodeType;
FILE* input;
FILE* output;
MemberNodeType head;
MemberNodeType member;
int main (void)
{
int authorNum, textNum, i, j;
member = malloc(sizeof(struct MemberNodeStruct));
head = member;
member->next = NULL;
input = fopen("input.txt", "r");
if (input == NULL)
{
printf("Could not open file.\n");
return 1;
}
fscanf(input, "%d", &authorNum);
for (i = 0; i < authorNum; i++)
{
member->anAuthor = malloc(sizeof(AuthorType));
textNum = 0;
if(fscanf(input, "%s %s %d", member->anAuthor->lastName, member->anAuthor->firstName, &member->anAuthor->idNumber) != EOF)
{
fscanf(input, "%d", &member->anAuthor->textNum);
for (j = 0; j < member->anAuthor->textNum; j++)
{
member->anAuthor->text = malloc(sizeof(struct AuthorRecord));
fscanf(input, " %[^\n]", member->anAuthor->text->textTitle);
fscanf(input, "%ld", &member->anAuthor->text->Download);
member->anAuthor->text = member->anAuthor->text->next;
}
member->next = malloc(sizeof(MemberNodeType));
member = member->next;
}
else
member->next = NULL;
}
member = head;
while(true)
{
if (member->next == NULL)
break;
for (i = 0; i < authorNum; i++)
{
printf("%s %s %d\n", member->anAuthor->lastName, member->anAuthor->firstName, member->anAuthor->idNumber);
for (j = 0; j < member->anAuthor->textNum; j++)
{
printf("%s\n", member->anAuthor->text->textTitle);
printf("%ld\n", member->anAuthor->text->Download);
member->anAuthor->text = member->anAuthor->text->next;
}
member = member->next;
}
}
}
I have run the program through gdb and everything is read fine. However, when I get to this line printf("%s\n", member->anAuthor->text->textTitle);
I get a segfault.
Apparently the member->anAuthor->text
field is empty, its address is 0x0. The data and memory addresses on member
and member->anAuthor
are correct.
What is wrong here?
Upvotes: 0
Views: 98
Reputation: 16540
There are several items to note in your code,
such as the definition of struct { fields } name; is depreciated
and should not be used in new code.
Rather use: struct name { fields };
which does require using 'struct' wherever the structure name is referenced.
There are several logic errors in the code that are causing it to fail,
such as not initializing the pointers to malloc'd areas to null.
Note: all the malloc'd areas need to be free'd at each exit point,
without back links, the free processing will be time consuming
Suggest adding a back link to each malloc'd area to simplify that operation
Also, the struct MemberNode is not needed
because a linked list of the struct AuthorType would contain all the relevant data.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
struct AuthorRecord
{
char textTitle[100];
int NumberOfWords;
long Download;
struct AuthorRecord *next;
};
struct AuthorType
{
char firstName[30];
char lastName[30];
int idNumber;
int textNum;
struct AuthorRecord text;
};
struct MemberNode
{
struct AuthorType *anAuthor;
struct MemberNode *next;
};
FILE* input;
struct MemberNode *head = NULL; // head of linked list
struct MemberNode *member = NULL; // ptr to current item in linked list
int main (void)
{
int authorNum, textNum, i, j;
input = fopen("input.txt", "r");
if (NULL == input)
{
perror("fopen(input.txt) %s", strerror(errno) );
return (errno);
}
// read number of authors
fscanf(input, "%d", &authorNum);
// for each author
for (i = 0; i < authorNum; i++)
{
// one of member struct per author
if( NULL == member )
{ // first time
member = (struct MemberNode*)malloc(sizeof(struct MemberNode));
head = member;
member->next = NULL; // initialize
}
else
{ // not first time
// link to new member node
member->next = (struct MemberNode*)malloc(sizeof(struct MemberNode);
if( NULL == member->next )
{
perror( "malloc(MemberNode) %s", strerror( errno ) );
return(errno);
}
//memset( member->next, 0x00, sizeof(struct MemberNode) );
member = member->next; // step to new member node
member->anAuthor = NULL; // initialize ptr to author info list
member->next = NULL; // initialize
}
// set secondary linked list member(author) & point to it
member->anAuthor = (struct AuthorType*)malloc(sizeof(struct AuthorType));
if( NULL == member->anAuthor )
{
perror( "malloc(AuthorType) %s", strerror( errno ) );
return(errno);
}
//memset( member->anAuthor, 0x00, sizeof(struct AuthorType) );
member->anAuthor->next = NULL; // initialize
member->anAuthor->text = NULL; // initialize
// read a line of author info
if(fscanf(input, "%s %s %d",
member->anAuthor->lastName,
member->anAuthor->firstName,
&(member->anAuthor->idNumber) ) != EOF)
{ // then, successful read of author info
// read number of author titles
fscanf(input, "%d", &(member->anAuthor->textNum));
// for each author title
for (j = 0; j < member->anAuthor->textNum; j++)
{
member->anAuthor->text =
(struct AuthorRecord*)malloc(sizeof(struct AuthorRecord));
if( NULL == member->anAuthor->text )
{
perror( "malloc(AuthorRecord) %s", strerror( errno ) );
return(errno);
}
//memset( member->anAuthor->text, 0x00, sizeof(struct AuthorRecord) );
// step to new author/text record
member->anAuthor->text = member->anAuthor->text->next;
member->anAuthor->text->next = NULL; // initialize
// read one title and how many times that title has been downloaded
fscanf(input, " %[^\n]", member->anAuthor->text->textTitle );
fscanf(input, "%ld", &(member->anAuthor->text->Download) );
}
}
}
// recall beginning of linked list
member = head;
//for each entry in linked list
while(NULL != member) // allow for empty list or end of list
{
if( NULL != member->anAuthor )
{
printf("%s %s %d\n",
member->anAuthor->lastName,
member->anAuthor->firstName,
member->anAuthor->idNumber);
// for each author/text
while( NULL != member->anAuthor->text->next )
{
printf("%s\n%ld\n",
member->anAuthor->text->textTitle,
member->anAuthor->text->Download);
// step to next author text/title
member->anAuthor->text = member->anAuthor->text->next;
}
}
// step to next member (I.E next author)
member = member->next;
}
}
Upvotes: 0
Reputation: 29116
When you read the texts for each author, you use the text
member of the autor node as iterator
for (j = 0; j < member->anAuthor->textNum; j++)
{
member->anAuthor->text = malloc(sizeof(struct AuthorRecord));
fscanf(input, " %[^\n]", member->anAuthor->text->textTitle);
fscanf(input, "%ld", &member->anAuthor->text->Download);
member->anAuthor->text = member->anAuthor->text->next;
}
The logic of this whole block is wrong. text->next
never has a meaningful value, not even NULL
. And the texts are not linked, they are just unrelated chunks of data, that once the loop has been traversed are unreachable. In particular, you must allocate first, then set the previous link's next
pointer to that memory.
The text
pointer of the text list should be like the head
in your member list and should only be assigned once. (After its initialisation with NULL
, that is.)
That said, there are various "smells" in your code:
AuthorRecord
when they are clearly different from AutorType
?AuthorRecordType
, not AuthorPtr
or PAuthor
. In C, a star marks a pointer type; it's hard to get more expressive than that. (Plus, it's easier to type.)next
pointers) to avoid possible incosistencies.malloc
when you need memory, not in advance. If you malloc the first element before reading it and then set it to NULL
because it turns out you didn't need it afer all, you create a memory leak.malloc
needs a free
after you're done. (I know that your code isn't complete at the moment; just saying.)scanf
sequence needs error checking. It's easy to get into an infinite loop if the input is not correct. (Side note: It's also helpful to post a short snippet of example input here to help us find the error.)main
. It is also a good idea to separate reading from creating your structures.member->anAuthor->text->next
. Consider using temporary pointers. This will make the references shorter (text->next
) and hopefully also the code cleaner.Upvotes: 1