Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

UPDATE

This post has been closed because my program actually had an error even though it worked in GDB so technically it wasn't meant for reviewing. However, after the help of some of friends here, I managed to fix my code and I've posted it below in the UPDATE section. I still left the ORIGINAL message though.

#include <stdio.h>  /* used for printf, and FILE struct */
#include <assert.h> /* used for debuging */
#include <stdlib.h> /* malloc and free are defined here */
#include <string.h> /* string includes strdup, duplicates a string */
#include <errno.h>  /* error header */


/* Structs */
struct Address {
    long id;
    int set;
    char *name;
    char *email;
};

/* struct rows is actually allocated on the stack. */
struct Database {
    long MaxRows;
    long MaxData;
    struct Address *rows;
};

/*
 * FILE object are usually created by fopen or tmpfile which both return a reference to
 * these objects. both of these functions allocate memory, so use fclose to free the resources
 */
struct Connection {
    FILE *file;
    struct Database *db;
};


/* Prototypes */
void Database_close(struct Connection *conn);
void die(const char *message, struct Connection *conn);
void Address_print(struct Address *addr);
void Database_load(struct Connection *conn);
struct Connection* Database_open(const char *filename, char mode);
void Database_write(struct Connection *conn);
void Database_create(struct Connection *conn, long in_maxRow, long in_maxData);
void Database_set(struct Connection *conn, int id, const char *name, const char *email);
void Database_get(struct Connection *conn, int id);
void Database_delete(struct Connection *conn, int id);
void Database_list(struct Connection *conn);





/* implementations */
struct Connection* Database_open(const char *filename, char mode)
{
    //allocate memory for a connection struct
    struct Connection *conn = malloc(sizeof(struct Connection));
    if(!conn) die("Memory error", conn);

    //allocate memory for db variable
    conn->db = malloc(sizeof(struct Database));
    if(!conn->db) die("Memory error", conn);

    //open the file 
    if(mode == 'c') {
        conn->file = fopen(filename, "w");

        if(conn->file) {    
            conn->db->MaxRows = 1;
            conn->db->MaxData = 1;
        }   

    } else {
        conn->file = fopen(filename, "r+");

        if(conn->file) {
            Database_load(conn);
        }
    }
    if(!conn->file) die("Failed to open the file", conn);

    return conn;   
}


void Database_load(struct Connection *conn)
{
    //set the position indicator with the stream to the begining of the file
    rewind(conn->file);

    int rc = fread(&conn->db->MaxRows, sizeof(long), 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

    rc = fread(&conn->db->MaxData, sizeof(long), 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

    //allocate memory based on the size extracted from the file
    conn->db->rows = malloc(sizeof(struct Address) * conn->db->MaxRows);
    if(!conn->db->rows) die("Memory error", conn);

    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        conn->db->rows[t_i].name = malloc(sizeof(char) * conn->db->MaxData);
        if(!conn->db->rows[t_i].name) die("Memory error", conn);

        conn->db->rows[t_i].email = malloc(sizeof(char) * conn->db->MaxData);
        if(!conn->db->rows[t_i].email) die("Memory error", conn);
    }

    //write each address to the file
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        rc = fread(&conn->db->rows[t_i].id, sizeof(long), 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fread(&conn->db->rows[t_i].set, sizeof(int), 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fread(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fread(conn->db->rows[t_i].name, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);    
    }  

}

void Database_create(struct Connection *conn, long in_maxRow, long in_maxData)
{
    //allocate memory for rows
    conn->db->rows = malloc(sizeof(struct Address) * in_maxRow);
    if(!conn->db->rows) die("Memory error", conn);

    int t_i = 0;
    for(t_i = 0 ; t_i < in_maxRow ; t_i++)
    {
        conn->db->rows[t_i].id = t_i;
        conn->db->rows[t_i].set = 0;

        conn->db->rows[t_i].name = malloc(sizeof(char) * in_maxData);
        if(!conn->db->rows[t_i].name) die("Memory error", conn);

        conn->db->rows[t_i].email = malloc(sizeof(char) * in_maxData);
        if(!conn->db->rows[t_i].email) die("Memory error", conn);
    }

    conn->db->MaxData = in_maxData;
    conn->db->MaxRows = in_maxRow;
}

void Database_write(struct Connection *conn)
{
    //set the position indicator with the stream to the begining of the file
    rewind(conn->file);

    //write the MaxRow size into to the file
    int rc = fwrite(&conn->db->MaxRows, sizeof(long), 1 , conn->file);
    if(rc != 1) die("Failed to write database.", conn);

    //write the MaxData size into to the file
    rc = fwrite(&conn->db->MaxData, sizeof(long), 1 , conn->file);
    if(rc != 1) die("Failed to write database.", conn);

    //write each address to the file
    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        rc = fwrite(&conn->db->rows[t_i].id, sizeof(long), 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fwrite(&conn->db->rows[t_i].set, sizeof(int), 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fwrite(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fwrite(conn->db->rows[t_i].name, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);    
    }

    //write unwritten data in the output buffer to the file
    rc = fflush(conn->file);
    if(rc == -1) die("Cannot flush database", conn);
}

void Database_close(struct Connection *conn)
{
    int t_i = 0;

    if(conn) {
        if(conn->file) fclose(conn->file);
        if(conn->db->rows) {
            for(t_i = 0; t_i < conn->db->MaxRows ; t_i++) {
                if(conn->db->rows[t_i].name) free(conn->db->rows[t_i].name);   
                if(conn->db->rows[t_i].email) free(conn->db->rows[t_i].email); 
            }
            free(conn->db->rows);
        }
        if(conn->db) free(conn->db);
        free(conn);
    }  
}

void die(const char *message, struct Connection *conn)
{
    if(errno) {
        perror(message);
    } else {
        printf("ERROR: %s\n", message);
    }

    /*close the connection*/
    Database_close(conn);
    exit(1);
}

void Address_print(struct Address *addr)
{
    printf("%d %s %s\n", addr->id, addr->name, addr->email);
}



void Database_set(struct Connection *conn, int id, const char *name, const char *email)
{
    //get the element at the specified id  
    struct Address *addr = &conn->db->rows[id];

    if(addr->set == 1) die("Already set, delete it first", conn);

    //set the set bit to 1
    addr->set = 1;

    memset(addr->name, '\0', conn->db->MaxData);
    memset(addr->email, '\0', conn->db->MaxData);

    char *res = strncpy(addr->name, name, strlen(name) + 1);
    if(!res) die("Name copy failed", conn);

    res = strncpy(addr->email, email, strlen(email) + 1);
    if(!res) die("Email copy failed", conn);

}

void Database_get(struct Connection *conn, int id)
{

    struct Address *addr = &conn->db->rows[id];

    if(addr->set) {
        Address_print(addr);
    } else {
        die("ID is not set", conn);
    }
}

void Database_delete(struct Connection *conn, int id)
{
    conn->db->rows[id].id = id;
    conn->db->rows[id].set = 0;
}

void Database_list(struct Connection *conn)
{
    int i = 0;
    struct Database *db = conn->db;

    for(i = 0; i< db->MaxRows; i++) {
        struct Address *cur = &db->rows[i];

        if(cur->set) {
            Address_print(cur);
        }
    }
}



int main(int argc, char *argv[])
{
    if(argc < 3) die("USAGE: lesson_9 <dbfile> <action> [action params]",NULL);  

    long _maxRow, _maxData = 0;
    char *filename = argv[1]; /* the first argument is the filename */
    char action = argv[2][0]; /* argv[2] is a string and argv[2][0] is action  */

    struct Connection *conn = Database_open(filename, action);

    int id = 0;

    if(argc >= 6) id = atoi(argv[3]);
    if(id >= conn->db->MaxRows) die("There not that many records", conn);

    switch(action) {
        case 'c':
                _maxRow = atoi(argv[3]);
                _maxData= atoi(argv[4]);
                Database_create(conn, _maxRow, _maxData);
                Database_write(conn);
                break;

        case 'g':
             if(argc != 4) die("Need an id to get", conn);
              Database_get(conn, id);
             break;

        case 's':
             if(argc != 6) die("Need id, name, email to set", conn);

             Database_set(conn, id, argv[4], argv[5]);
             Database_write(conn);
             break;

        case 'd':
             if(argc != 4) die("Need id to delete", conn);

             Database_delete(conn, id);
             Database_write(conn);
             break;

        case 'l':
             Database_list(conn);
             break;

        default:
             die("Invalid action, only: c=create, g=get, s=set, d=del, l=list", conn);
    }

    Database_close(conn);

    return 0;
}


ORIGINAL MESSAGE

I've been following Zed Shaw's LearnCtheHardWay series and I got stuck on Chapter 18 Exercise 17: Heap And Stack Memory Allocation Extra credit exercise number 2. Here is the link to the page: http://c.learncodethehardway.org/book/learn-c-the-hard-waych18.html

The program basically creates a flat-file database and read and writes to it using a fixed size array. Now basically I have to change the code to allow the user to input Row and Data size, which will dynamically allocated instead of using arrays on the stack. The program runs fine from GDB but I get a segmentation fault when I run it from the terminal. So the program works, and it doesn't. That is why I posted the question here. The segmentation fault happens inside the Database_load function, the line which says:

rc = fread(conn->db->rows, sizeof(struct Address) * conn->db->MaxRows, 1, conn->file);

to run the app and see the seg fault, first create a database:

$ ./fileDb db.dat 3 100 c

then try to set a value

$ ./fileDb db.dat 3 100 s 1 zed [email protected]

and here is the code

/**************************
*        fileDb.c         *     
***************************/

/*
 * Extra Credit
 * ============
 *
 */


#include <stdio.h>  /* used for printf, and FILE struct */
#include <assert.h> /* used for debuging */
#include <stdlib.h> /* malloc and free are defined here */
#include <string.h> /* string includes strdup, duplicates a string */
#include <errno.h>  /* error header */

#define MAX_DATA 512
#define MAX_ROWS 100

struct Address {
    long id;
    int set;
    char *name;
    char *email;
};

/*
 * struct rows is actually allocated on the stack.
 */
struct Database {
    long MaxRows;
    long MaxData;
    struct Address *rows;
};

/*
 * FILE object are usually created by fopen or tmpfile which both return a reference to
 * these objects. both of these functions allocate memory, so use fclose to free the resources
 */
struct Connection {
    FILE *file;
    struct Database *db;
};

void Database_close(struct Connection *conn)
{
    int t_i = 0;

    if(conn) {
        if(conn->file) fclose(conn->file);
        if(conn->db->rows) {
            for(t_i = 0; t_i < conn->db->MaxRows ; t_i++) {
                if(conn->db->rows[t_i].name) free(conn->db->rows[t_i].name);   
                if(conn->db->rows[t_i].email) free(conn->db->rows[t_i].email); 
            }
            free(conn->db->rows);
        }
        if(conn->db) free(conn->db);
        free(conn);
    }  
}

void die(const char *message, struct Connection *conn)
{

/*
 * when there is an error returned from a function, it will set an external variable
 * called errno to say exactly what error happened. The errno is just a number, and
 * the perror function will print the error message of that number.
 */

    if(errno) {
        perror(message);
    } else {
        printf("ERROR: %s\n", message);
    }

    /*close the connection*/
    Database_close(conn);
    exit(1);
}

void Address_print(struct Address *addr)
{
    printf("%d %s %s\n", addr->id, addr->name, addr->email);
}

void Database_load(struct Connection *conn)
{
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

    rc = fread(conn->db->rows, sizeof(struct Address) * conn->db->MaxRows, 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

    //write each address to the file
    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        rc = fread(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fread(conn->db->rows[t_i].name, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);    
    }  

}

struct Connection* Database_open(const char *filename, char mode, long in_maxData, long in_maxRow)
{
    //allocate memory for a connection struct
    struct Connection *conn = malloc(sizeof(struct Connection));
    if(!conn) die("Memory error", conn);

    //allocate memory for db variable
    conn->db = malloc(sizeof(struct Database));
    if(!conn->db) die("Memory error", conn);

    //set the max row and data numbers
    conn->db->MaxRows = in_maxRow;
    conn->db->MaxData = in_maxData;

    /*
    * allocate memory for the rows variable
    * rows is an array of size conn->db->MaxRows, and each a size of struct Address type
    * so conn->db->rows[1] is a struct Address type, in which two variables of the type char* need to
    * have memory allocated for them. for each of these pointers, we need to allocate
    */
    conn->db->rows = malloc(sizeof(struct Address) * in_maxRow);
    if(!conn->db->rows) die("Memory error", conn);

    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        conn->db->rows[t_i].name = malloc(sizeof(char) * conn->db->MaxData);
        if(!conn->db->rows[t_i].name) die("Memory error", conn);

        conn->db->rows[t_i].email = malloc(sizeof(char) * conn->db->MaxData);
        if(!conn->db->rows[t_i].email) die("Memory error", conn);
    }

    if(mode == 'c') {
        conn->file = fopen(filename, "w");
    } else {
        conn->file = fopen(filename, "r+");

        if(conn->file) {
            Database_load(conn);
        }
    }

    if(!conn->file) die("Failed to open the file", conn);

    return conn;   
}

void Database_write(struct Connection *conn)
{
    //set the position indicator with the stream to the begining of the file
    rewind(conn->file);

    /*
     * cannot write the whole structure in a single write.
     * The memory is allocated by traversing down the structure path
     * and allocating one pointer at a time. This doens't garentee that
     * the whole data is in a single continues block
     */
     //int rc = fwrite(conn->db, t_dbSize, 1, conn->file);

    //write the database to the file
    int rc = fwrite(conn->db, sizeof(struct Database), 1 , conn->file);
    if(rc != 1) die("Failed to write database.", conn);

    rc = fwrite(conn->db->rows, (sizeof(struct Address) * conn->db->MaxRows), 1 , conn->file);
    if(rc != 1) die("Failed to write database.", conn);

    //write each address to the file
    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        rc = fwrite(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fwrite(conn->db->rows[t_i].name, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);    
    }

    //any unwritten data in the output buffer is written to the file
    //the stream shall be flushed after an output operation before
    //performing an input operation
    rc = fflush(conn->file);
    if(rc == -1) die("Cannot flush database", conn);
}

void Database_create(struct Connection *conn)
{
    int i = 0;
/*
 * for all the rows in the database, create a struct object addr, and assign
 * default values as means of initialization
 */
    for(i = 0 ; i < conn->db->MaxRows; i++) {
        conn->db->rows[i].id = i;
        conn->db->rows[i].set = 0;
    }
}

void Database_set(struct Connection *conn, int id, const char *name, const char *email)
{

    printf("id:%d , name:%s , email:%s \n", id, name, email);

    //get the element at the specified id  
    struct Address *addr = &conn->db->rows[id];

    printf("addr address :%p \n", addr);
    printf("id: %d ", addr->id);
    printf("set: %d ", addr->set);
    printf("name: %s ", addr->name);
    printf("email: %s \n", addr->email);


    if(addr->set == 1) die("Already set, delete it first", conn);


    printf("after addr->set is called");

    //set the set bit to 1
    addr->set = 1;

    /*copy name to addr->name
    *char *strncpy(char *restrict s1, const char *restrict s2, size_t n);
    *strncpy shall copy not more than n bytes, (all bytes after a null byte are not copied)
    *from s2, to s1. if size of s2 is less than n, then null bytes are appended to the end
    * of s1 until all n bytes are written.
    *
    * strncpy, doens't crash if name or addr->name is greater than MAX_DATA.
    * Ask Zed about this
    */

    printf("before memset is called \n");

    memset(addr->name, '\0', conn->db->MaxData);
    memset(addr->email, '\0', conn->db->MaxData);

    printf("memset is called \n");

    char *res = strncpy(addr->name, name, strlen(name) + 1);
    if(!res) die("Name copy failed", conn);

    printf("strncpy is called \n");

    res = strncpy(addr->email, email, strlen(email) + 1);
    if(!res) die("Email copy failed", conn);

    printf("strncpy email is called \n");
}

void Database_get(struct Connection *conn, int id)
{

    struct Address *addr = &conn->db->rows[id];

    if(addr->set) {
        Address_print(addr);
    } else {
        die("ID is not set", conn);
    }
}

void Database_delete(struct Connection *conn, int id)
{
    conn->db->rows[id].id = id;
    conn->db->rows[id].set = 0;
}

void Database_list(struct Connection *conn)
{
    int i = 0;
    struct Database *db = conn->db;

    for(i = 0; i< db->MaxRows; i++) {
        struct Address *cur = &db->rows[i];

        if(cur->set) {
            Address_print(cur);
        }
    }
}



int main(int argc, char *argv[])
{
    if(argc < 5) die("USAGE: lesson_9 <dbfile> <Max_rows> <Max_data>  <action> [action params]",NULL);  

    char *filename = argv[1]; /* the first argument is the filename */
    char action = argv[4][0]; /* argv[2] is a pointer to a char and argv[2][0] is the first char  */
    long t_max_row = atoi(argv[2]);
    long t_max_data= atoi(argv[3]);

    struct Connection *conn = Database_open(filename, action,t_max_data,t_max_row);

    printf("Con->db detailsL MaxRows=%d MaxData=%d \n",
                 conn->db->MaxRows,
                 conn->db->MaxData);

    int id = 0;

    if(argc > 5) id = atoi(argv[5]);
    if(id >= t_max_row) die("There not that many records", conn);

    switch(action) {
        case 'c':
            Database_create(conn);
            Database_write(conn);
            break;

        case 'g':
         if(argc != 4) die("Need an id to get", conn);
          Database_get(conn, id);
         break;

      case 's':
         if(argc != 8) die("Need id, name, email to set", conn);

         Database_set(conn, id, argv[6], argv[7]);
         printf("Database_set");
            Database_write(conn);
         break;

      case 'd':
         if(argc != 4) die("Need id to delete", conn);

         Database_delete(conn, id);
         Database_write(conn);
         break;

      case 'l':
         Database_list(conn);
         break;

      default:
         die("Invalid action, only: c=create, g=get, s=set, d=del, l=list", conn);
    }

    Database_close(conn);

    return 0;
}
share|improve this question

closed as off topic by Michael K Jun 12 '12 at 19:58

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

2  
Ah, the infamous Heisenbug. Usually they are caused by racing conditions (unlikely here) or an invalid read. Have you tried putting in some debug printf's to narrow down the error? Valgrind might be able to help, too. –  Benjamin Kloster Jun 11 '12 at 20:46
    
@BenjaminKloster: Heisenbug, from breaking bug. I like the sound of that. Yes I forgot to mention where the seg fault happens. It's inside the Database_load function, the line which says: [code]rc = fread(conn->db->rows, sizeof(struct Address) * conn->db->MaxRows, 1, conn->file);[/code] –  Armen B. Jun 11 '12 at 22:14
add comment

3 Answers

up vote 3 down vote accepted

Running this through valgrind gives me

==26302== Invalid write of size 8
==26302==    at 0x4EBB891: __GI_memcpy (memcpy.S:196)
==26302==    by 0x4EA5DDC: _IO_file_xsgetn (fileops.c:1414)
==26302==    by 0x4E9A2D2: fread (iofread.c:44)
==26302==    by 0x400B96: Database_load (database.c:94)
==26302==    by 0x400EBE: Database_open (database.c:149)
==26302==    by 0x4014FD: main (database.c:303)
==26302==  Address 0x16e4050 is not stack'd, malloc'd or (recently) free'd

So it seems you're writing data where you shouldn't have. Putting in some debug statements like so:

void Database_load(struct Connection *conn)
{
    printf("conn->db->rows before first read: %p\n", (void*) conn->db->rows);
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
    printf("conn->db->rows after first read: %p\n", (void*) conn->db->rows);
    ...

Yields the output

conn->db->rows before first read: 0x51d00f0
conn->db->rows after first read: 0x16e4050

So blufox's answer above is correct. You're overwriting the rows pointer. Changing the fread line to this:

int rc = fread(conn->db, 2 * sizeof(long), 1, conn->file);

seems to read out MaxRows and MaxData correctly, but segfaults a few lines later in the for-loop:

rc = fread(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);

Which stems from the same problem, since in the second fread call:

rc = fread(conn->db->rows, sizeof(struct Address) * conn->db->MaxRows, 1, conn->file);

You overwrite the Address structs' name and email pointers with something from the file.

I suggest you review your data model to make sure you're only writing and reading data that is actually persistent, and not some volatile values like pointers.

share|improve this answer
1  
Ok Now I understand. I'm overwriting the pointer's value (changing the address which it is pointing to). And I should never write pointer address to a file! The reason why it worked in GDB it's because when the pointer value was written to the file- and then loaded back- there were still data in that memory located which the loaded pointer value was pointing at. If I ran the create database on a different GDB session, then I would've seen my bug. Thank you, you also taught me a little bit more about valgrind and how to use the info to debug software. You 'C'ule man. –  Armen B. Jun 12 '12 at 17:34
add comment
void Database_load(struct Connection *conn)
{
    int rc = fread(conn->db, sizeof(struct Database), 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

So, doesn't this overwrite your current value of conn->db->rows (that you allocated earlier) with the nonsensical value that is saved in the file?

    rc = fread(conn->db->rows, sizeof(struct Address) * conn->db->MaxRows, 1, conn->file);
    if(rc != 1) die ("Failed to load database.", conn);

    //write each address to the file
    int t_i = 0;
    for(t_i = 0 ; t_i < conn->db->MaxRows ; t_i++)
    {
        rc = fread(conn->db->rows[t_i].email, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);

        rc = fread(conn->db->rows[t_i].name, sizeof(char) * conn->db->MaxData, 1, conn->file);
        if(rc != 1) die("Failed to write database.", conn);    
    }  

}
share|improve this answer
    
@bluefox: thanks for pointing out my error. I didn't understand your statement well, but after BKloster spoon fed me your statement. –  Armen B. Jun 12 '12 at 17:40
2  
@ArmenB. :), The error was difficult to spot, but once spotted, seemed trivial. That is why I didn't explain it further. Btw, even though it didn't work in this case, you might learn to use cbmc which shows you most pointer related and bounds checking errors. –  rahul Jun 12 '12 at 17:46
add comment

Since you're trying to learn C the hard way :)

The problem is that you're reading and then using an essentially random variable - rows (and name, email) needs to be allocated when you load, you can't reuse pointers. If you want to do it this way, you should have structures such as:

struct DatabaseHeader {
    long MaxRows;
    long MaxData;
};

struct Database {
    DatabaseHeader header;
    struct Address *rows;
};

In the database_load function, read the header first.

int rc = fread(&conn->db.header, sizeof(struct DatabaseHeader), 1, conn->file);

You now know what the size (MaxRows) is, so allocate that:

int address_size = sizeof (struct Address);
conn->db.rows = malloc( conn->db.header.MaxRows*address_size );

Unfortunately, you'll have to do this for the email and name fields as well, which rather complicates things, since you'll have to read things in one at a time.

int field_width = conn->db.header.MaxData*sizeof (char);
for ( i = 0; i < conn->db.rows.header.MaxRows; i++ )
{
    struct Address * addr = &conn->db.rows[i];
    rc = fread(addr, sizeof(struct Address), 1, conn->file);

    addr->name = malloc( field_width );
    rc = fread(addr->name, field_width, 1, conn->file);

    addr->email = malloc( field_width );
    rc = fread(addr->email, field_width, 1, conn->file);
}

*Warning: This will not work as written! You'll have to make adjustments (to this or other functions) to avoid memory leaks (there's a conflict between this and database_open).*

share|improve this answer
    
Thanks for the alternative solution Glenn. I understood why you placed the two values in a separate struct, so I don't have to load the two values twice, right? int rc = fread(&conn->db.MaxData, sizeof(long), 1, conn->file); and int rc = fread(&conn->db.MaxRow, sizeof(long), 1, conn->file); –  Armen B. Jun 12 '12 at 17:37
    
That's correct. Nothing wrong with doing it separately, but it may be easier to read/understand later if you group it. –  Glenn Rogers Jun 13 '12 at 8:55
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.