C - Struct to Pointer

Question

All of the correct values are printed when I print from within my read function, but when I return a Computer* then most of the values are lost for some reason. Note that I cannot change the function headers, and I cannot use global variables.

Read Function:

Computer* readComputers( char* filename ) {

FILE* fp = fopen( filename, "r" );

if( fp == NULL ) {
    printf( "Error: Invalid file!" );
    exit( 0 );
}

Computer computer[5];
Computer *compPtr;
int i;

for( i = 0; i < 5; i++ ) {
    fscanf( fp, "%s %i", computer[i].serialNumber, &computer[i].price );
    fscanf( fp, "%i %i", &computer[i].GBofRAM, &computer[i].GBofHardDrive );
    if( feof( fp ) ) {
        break;
    }
}

compPtr = &computer;

for( i = 0; i < 5; i++ ) {

/*
*   THIS WORKS TO PRINT OUT ANY VALUE I WANT USING THE POINTER
    printf( "%s\n", (compPtr + i)->serialNumber );
*/

}

fclose( fp );

return compPtr;

}

Main Function (where I return the pointer to):

    Computer *computers;

    computers = readComputers( filename );

    for( i = 0; i < 5; i++ ) {
        printf( "%s\n", (computers + i)->serialNumber );  
    }

When I return the pointer from readComputers the values are getting messed up, and disappearing when trying to print from the main function.

Answer 1

You return a pointer to a structure with automatic storage. This structure is a local variable stored on the stack (most likely) and is not preserved after the function returns. It gets clobbered by local data from other functions called after readComputers returns.

This is a very basic bug that the compiler should be able to detect but does not, either because you do not ask for enough warnings (try gcc -Wall -W) or because you return the pointer via a local variable and not the array itself.

You can correct this by allocating the structure you return with malloc or calloc or by using external storage and passing a pointer to that to readComputers. The first approach can be implemented this way:

Remove the definition of computer and compPtr and write this:

Computer *computer = calloc(sizeof(Computer), 5);

and return computer instead of compPtr.

There as other troubling problems in your code: using fscanf to parse input is risky because the semantics of this family of functions is subtile and error-prone. For example:

fscanf( fp, "%s %i", computer[i].serialNumber, &computer[i].price );

• you do not check the return value, which is the only reliable way to know how many fields were correctly parsed.
• you do not tell fscanf the size of the array computer[i].serialNumber. Reading invalid input will cause undefined behaviour, it may cause your program to crash or could even be used by an attacker to take control of the program/computer...
• The check on feof(fp) after the calls to fscanf is incorrect: it does not tell whether to fields were correctly parsed or not.
• Furthermore, you have no way to return to the caller how many structures were correctly parsed. Your API needs some improvements.

Answer 2

Note that the pointer you are returning points to an array which is allocated in the readComputers function's scope, meaning it is deallocated once the function ends. Using such a pointer is not a good idea. Instead you can allocate an array in your main function, and pass its pointer to your read computers function (as well as its size and proper validation in your function for preventing index out of bounds on your array). You can then safely read your data into the Computer struct/class array.

Answer 3

Following on from the discussion in the comments, an example is probably the easiest way to sew up the loose ends. While declaring the memory as a block to hold your 5 computers is fine, you will find in more complex situations, that declaring x number of pointers-to-pointers and then allocating space for each computer and assigning that value to the pointer makes more sense. (it is what allows the computers[i] reference later on).

However, for this example, simply declaring a block big enough to hold 5 is fine. Try the following, and if you have further questions, just let me know:

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

#define MAXSN 24
#define NCOMP 5

typedef struct {
    char serialNumber[MAXSN];
    int price;
    int GBofRAM;
    int GBofHardDrive;
} Computer;

Computer *readComputers (char *filename);

int main (int argc, char **argv) {

    Computer *computers = NULL;
    int i = 0;

    if (argc < 2) {
        fprintf (stderr, "error: insufficient input. usage %s filename\n",
                argv[0]);
        return 1;
    }

    computers = readComputers ( argv[1] );

    if (!computers) {
        fprintf (stderr, "error: no computers read from file\n");
        return 1;        
    }

    printf ("\n No. serialNumber  price GBofRAM GBofHardDrive\n");
    printf (" --- ------------ ------ ------- -------------\n");
    while ((computers + i)-> serialNumber && i < NCOMP) {
        printf ( " (%d) %12s %6d %7d %13d\n", i,
                (computers + i)->serialNumber,
                (computers + i)-> price,
                (computers + i)-> GBofRAM,
                (computers + i)-> GBofHardDrive);
        i++;
    }

    free (computers);

    return 0;
}

Computer *readComputers (char* filename)
{
    FILE* fp = fopen( filename, "r" );

    if( !fp ) {
        printf( "Error: Invalid file!" );
        exit( 0 );
    }

    /* use calloc to allocate and initialize to 0 since you
       do not know how many you will read from file */
    Computer *compPtr = calloc ( 5, sizeof *compPtr );
    int i = 0;

    while (fscanf( fp, "%s %d %d %d",
                (compPtr + i)-> serialNumber,
                &(compPtr + i)-> price,
                &(compPtr + i)-> GBofRAM,
                &(compPtr + i)-> GBofHardDrive ) == 4 && i < 5) { i++; }

    fclose( fp );

    return compPtr;
}

Test Input

$ cat dat/comp.dat
14490 699 24 1000
29056 549 12 1000
19668 659 16 1000
13167 379 4   500
26531 499 8   750

Compile

gcc -Wall -Wextra -o bin/struct_rdcomputers struct_rdcomputers.c

The optimization level -Ofast or -O(0-3) [0 default] are optional optimization levels you can add that can greatly reduce runtime. However, for initial building and debugging you will want to minimize the use of optimization to insure the compiler doesn't optimize away information you may need to debug your code.

Output

$ ./bin/struct_rdcomputers dat/comp.dat

 No. serialNumber  price GBofRAM GBofHardDrive
 --- ------------ ------ ------- -------------
 (0)        14490    699      24          1000
 (1)        29056    549      12          1000
 (2)        19668    659      16          1000
 (3)        13167    379       4           500
 (4)        26531    499       8           750

Memory Check

When you allocate memory dynamically, you are responsible for (1) preserving a pointer to the begining of the block of memory allocated (so it can be freed); and (2) freeing the memory when it is no longer needed. You also need to insure you are using it correctly and there are no errors in the way you are accessing the memory. The use of valgrind (or similar tool) is simple to use, and essential, to validate you are freeing all memory you have allocated and are not using it in a way that will come back and bite you later. Simply type valgrind progname and then read the results:

$ valgrind ./bin/struct_rdcomputers dat/comp.dat
==8529== Memcheck, a memory error detector
==8529== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==8529== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==8529== Command: ./bin/struct_rdcomputers dat/comp.dat
==8529==

 No. serialNumber  price GBofRAM GBofHardDrive
 --- ------------ ------ ------- -------------
 (0)        14490    699      24          1000
 (1)        29056    549      12          1000
 (2)        19668    659      16          1000
 (3)        13167    379       4           500
 (4)        26531    499       8           750
==8529==
==8529== HEAP SUMMARY:
==8529==     in use at exit: 0 bytes in 0 blocks
==8529==   total heap usage: 2 allocs, 2 frees, 748 bytes allocated
==8529==
==8529== All heap blocks were freed -- no leaks are possible
==8529==
==8529== For counts of detected and suppressed errors, rerun with: -v
==8529== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

Answer 4

You should allocate memory in your method, like:

instead of:

compPtr = &computer;

you should:

compPtr = malloc( 5 * sizeof(Computer));
memcpy(compPtr, computer, 5 * sizeof(Computer));

And in your main(), after you print it out, you need to

free(computers);

Answer 5

The memory you are allocating within your function, is deallocated / freed after you exit the function. Thus accessing it via pointer after the function is finished, is a memory violation. Allocate your memory before you call the function and give a pointer with array length to the function

main:
#define COMPUTER_LEN 5
Computer computers[COMPUTER_LEN];   // allocate memory

void readComputers( filename, computers, COMPUTER_LEN);  // give pointer and length of array to function

//read from computers
...