user3484582
user3484582

Reputation: 557

Node in linked list appears to be empty when trying to print it

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

Answers (2)

user3629249
user3629249

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

M Oehm
M Oehm

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:

  • The structures are horribly named. I know that it's an assignment and your were given the structures as a starting point, but why are the text nodes called AuthorRecord when they are clearly different from AutorType?
  • As has been pointed out, defining pointer types for the structs adds nothing; it only confuses, espectally if the types are named 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.)
  • Keeping track of the number of entries in a list with an extra member can be useful, but the traversal of the list should always be with the list structure itself (i.e. the next pointers) to avoid possible incosistencies.
  • When you add elements to the list, call 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.
  • Every malloc needs a free after you're done. (I know that your code isn't complete at the moment; just saying.)
  • Your whole 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.)
  • Consider making small functions for creating each of the types, for cleaning them up and for adding them into the lists; don't put everything into main. It is also a good idea to separate reading from creating your structures.
  • You use long sequences of member references like this: member->anAuthor->text->next. Consider using temporary pointers. This will make the references shorter (text->next) and hopefully also the code cleaner.

Upvotes: 1

Related Questions