Storing strings using fgets

Question

I have a file which contains information about films like this:

Film code
Name
Year of release
Movie length(in minutes)
The film producer

I have to read this info from a file and store that info into pointers. My code so far:

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

typedef struct filmiab
{
    int koodpk;
    char *nimed;
    int aasta;
    int kestus;
    char *rezi;
} filmiab;

int main()
{
    filmiab *db;

    FILE *f1;
    f1 = fopen("filmid.txt", "r");

    db->nimed = (char*)malloc(sizeof(db->nimed) * sizeof(char));
    db->rezi = (char*)malloc(sizeof(db->rezi) * sizeof(char));

    while(1)
    {
        fscanf(f1, "%d ", &db->koodpk);
        fgets(db->nimed, 100, f1);
        db->nimed = (char*)realloc(db->nimed, sizeof(char) * sizeof(db->nimed)); //gets more memory to store the strings
        fscanf(f1, "%d %d ", &db->aasta, &db->kestus);
        fgets(db->rezi, 100, f1);
        db->rezi = (char*)realloc(db->rezi, sizeof(char) * sizeof(db->rezi));

        printf("Filmi kood: %d\nFilmi nimi: %sAasta: %d\nKestus minutites: %d\nFilmi rezis66r: %s\n",
        db->koodpk, db->nimed, db->aasta, db->kestus, db->rezi);
        printf("\n");
    }

    return 0;
}

It just goes into an infinte loop and only prints the last 5 lines. I know that when using fgets it replaces all the strings with the last 5 lines. But what can I do so it would store all the info and that so I could print them out (or just use them) in another function. And why does it go into an infinite loop ?

EDIT: I have to use only the pointers that are created in the struct.

EDIT2: Now both these lines fgets(db->nimed, 100, f1); fgets(db->rezi, 100, f1); store the required info and the blank spaces. What to do so it only stores the names of the films and the producers.

Answer 1

It just goes into an infinte loop

That's because it is an infinite loop. You have a while(1) with no break condition. It should break after it can no longer read any lines.

Every time you work with a file, that means fopen, fgets, and fscanf, you need to check whether the operation succeeded. If it fails, the code will continue with whatever garbage is the result.

This is especially a problem with fscanf because if it fails, it leaves the file pointer where it was, and might continuously rescan the same line over and over again. In general, avoid scanf and fscanf. Instead, fgets the whole line, to ensure it gets read, and scan it with sscanf.

---

The other problem is how you're allocating memory isn't right.

filmiab *db;

This puts a pointer on the stack, but it points to garbage. No memory has been allocated for the actual struct.

db->nimed = (char*)malloc(sizeof(db->nimed) * sizeof(char));

sizeof(db->nimed) is not the length of the string in db->nimed, but the size of the pointer. Probably 4 or 8. So you've only allocated 4 or 8 bytes.

fgets(db->nimed, 100, f1);

Then you read up to 100 bytes into it with fgets, probably causing a buffer overflow.

db->nimed = (char*)realloc(db->nimed, sizeof(char) * sizeof(db->nimed));

Then you reallocate too little, too late. Again, same as before, this is allocating only 4 or 8 bytes. It's probably doing nothing because the memory was already this size.

---

To fix this, start by putting the whole struct on the stack.

filmiab db;

Then allocate the necessary space for its strings. Note that since sizeof(char) is always 1 there's no need to include it. There's also no need to cast the result of malloc.

db.nimed = malloc(100);
db.rezi = malloc(100);

Now there's no need to realloc, you've got you 100 bytes of memory and can write to it with fgets.

---

For future reference, here's how I'd rework this.

int main() {
    filmiab db;

    char file[] = "filmid.txt";
    FILE *f1 = fopen(file, "r");
    if( f1 == NULL ) {
        fprintf( stderr, "Could not open %s for reading: %s", file, strerror(errno) );
    }

    char line[1024];
    int state = 0;

    while(fgets(line, 1024, f1) != NULL) {
        switch(state % 5) {
            case 0:
                sscanf(line, "%d", &db.koodpk);
                break;
            case 1:
                db.nimed = strdup(line);
                break;
            case 2:
                sscanf(line, "%d", &db.aasta);
                break;
            case 3:
                sscanf(line, "%d", &db.kestus);
                break;
            case 4:
                db.rezi = strdup(line);
                printf("Filmi kood: %d\nFilmi nimi: %sAasta: %d\nKestus minutites: %d\nFilmi rezis66r: %s\n",
                        db.koodpk, db.nimed, db.aasta, db.kestus, db.rezi);
                printf("\n");
                break;
            default:
                // Should never get here
                assert(0);
                break;
        }

        state++;
    }

    return 0;
}

There's a single, large line buffer which gets reused, it's 1K but it's only 1K once. strdup duplicates strings, but only allocates enough memory to hold the string. This eliminates the need to predict how big lines are, and it also avoids fragmenting memory with a lot of reallocs.

In this particular case, since db is being reused, it would be more optimal to just allocate 1024 each for db.nimed and db.rezi, but I wanted to demonstrate the more general case where the stuff read in will stick around.

while(fgets(line, 1024, f1) != NULL) ensures I'll read until the end of the file. Then line is processed using the switch statement depending on what sort of line is coming next. This separates the process of reading from a file, which can be unpredictable and needs a lot of error checking, from processing the data which is a bit easier. Technically I should be checking if those sscanfs succeeded, but I got lazy. :)