Malloc struct printing error in C

Question

I'm trying to write a program which reads a .bin file into dynamically allocated structs. It seems to be doing what I want, up until I need to print the results. I'm met with an endless loop of the same struct over and over again, instead of each struct just once. The code is as follows:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<time.h>

main(){
    //File pointer
    FILE *binFile = NULL;

    //Struct declaration
    typedef struct flightData_struct {
        char flightNum[7];
        char origCode[5];
        char destCode[5];
        int timeStamp;
        struct flightData_struct* next;
    } flightData;
    flightData myFlightData;
    struct flightData_struct *head;
    struct flightData_struct *tail;

    //Opening file
    printf("Opening file...\n");

    binFile = fopen("acars.bin", "rb");
    if (binFile == NULL){
        printf("ERROR: Could not open file. Please try again.");
        return -1;
    }

    //Malloc'ing first struct
    struct myflightData *point = (struct myFlightData*) malloc(sizeof(flightData));
    fread(point, sizeof(flightData), 1, binFile);

    head = point;
    tail = point;

    //Malloc'ing structs
    while (!feof(binFile)){
        flightData *temp = (struct flightData*) malloc(sizeof(flightData));
        fread(temp, sizeof(flightData), 1, binFile);
        temp->next = NULL;
        tail->next = temp;
        tail = tail->next;
    }

    tail = head;

    while (tail != 0){
        int t;
        t = tail->timeStamp;
        time_t time = t;
        printf("%s| %s| %s| %s\n\n", tail->flightNum, tail->origCode, tail->destCode, asctime(gmtime(&time)));
    }

    //Closing file
    printf("Closing file...");
    fclose(binFile);
    return 0;

}

And this is the output I get from it (it seems to loop infinitely).

If this loop `while (tail != 0)` once started, why would you expect it to end, with the value of `tail` never being changed inside the loop? — alk, Aug 01 '15 at 20:27
Oh! Now I see it, thanks. Though, now I'm not sure what I should do to work around that. Any suggestions? — Glyph, Aug 01 '15 at 20:30
As I assume this is homework: Have a look at how you add nodes to the list in the allocating loop. Also note you have the head stored. For printing better start with the head going from one node to the next until the `next` pointer is `NULL`. — alk, Aug 01 '15 at 20:34
And regardless of what some instructor told you, this: `while (!feof(binFile))` is wrong as you're using it. [**Read this**](https://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) to learn why. You're ignoring any read-failure returned from `fread` and assuming it always works. Assumption is the mother of all... — WhozCraig, Aug 01 '15 at 20:41

dbush · Answer 1 · 2015-08-02T12:34:14.257

In your while (tail != 0) loop, you're never updating tail, so the loop condition never changes. You need to move tail down the list. Add this to the end of the loop:

tail = tail->next;

That will allow you to traverse the list.

Edit:

While this works, you're losing a reference to the tail of the list, and it's confusing for others reading your code. You should instead define a separate variable such as flightData temp, initializing that to head, and using that in your loop instead of tail.

A few other issues:

Never cast the return value of malloc in C, as doing so can mask other issues such as stdlib.h not being included. Since malloc returns a void *, it can be assigned to any pointer type without a cast. Note that this is different than C++ which requires such a cast.

Then there's this:

struct myflightData *point = malloc(sizeof(flightData));

There's no struct myflightData defined. You probably wanted flightData instead.

You're also failing to check the return value of fread. You should be doing something like this:

    len = fread(temp, sizeof(flightData), 1, binFile);
    if (len == -1) {
        perror("fread failed");
        exit(1);
    } else if (len != 1) {
        fprintf(stderr,"read %d, expected %d\n", len, 1);
    }

"*That will allow you to traverse the list.*" I doubt this, as you start by the tail you'll only get the one last element of the list being the tail. — alk, Aug 02 '15 at 09:29
@alk Not when there's `tail = head;` just before the start of the loop. Granted, it's not the best use of variable names, but it works. — dbush, Aug 02 '15 at 11:22
Fair enough ... - I somehow overlooked the useless (*sigh*) loss of the reference to the tail by overwriting it with the head pointer .... Please touch your answer so I can undo my downvote. — alk, Aug 02 '15 at 11:28

Malloc struct printing error in C

1 Answers1