What is this bug about? Structs, Pointers, Dynamic Memory Allocation , C

Question

I'm writing a simple banking application in c It saves the information in a file. I want to load the file each time the app runs, and add the information from the file to the struct, for this purpose, I have written two functions called "loadfile" and "allocate"

In the function "loadfile" if I un-comment the comment lines, the OS throws a "stopped working" in my face :|

Can you help me with it ? when I use (acc+i) in the "loadfile", the error shows up. Is there a syntax problem? :o thanks

typedef struct {
    char name[20];
    int id;
    int balance;
    char branch[10];
} account;

account *acc;

int allocate ( account *acc )  {
    int num = 0 ;
    char tempname[20],tempbranch[10];
    int tempid = -1 ,tempbalance;
    FILE *file;
    file = fopen("D://bank.txt","r");
    while ( !feof(file) ) {
        fscanf(file,"%s %d %d %s ",tempname, &tempid, &tempbalance, tempbranch);
        if (tempid != -1)
            num++;
    }
    acc = ( account *) realloc ( acc, num * sizeof(account) );
    fclose(file);
    printf(" num in allocate function : %d",num);
    return num;
}

int loadfile (account *acc) {
    int num = allocate(acc);
    char tempname[20],tempbranch[10];
    int tempid ,tempbalance;
    if ( num != 0 ) {
        int i = 0 ;
        FILE *file;
        file = fopen("D:\\bank.txt","r+");
        for ( i = 0 ; !feof(file) && i < num ; i++ ) {
            fscanf(file,"%s ",tempname );
            fscanf(file,"%d ",&tempid );
            fscanf(file,"%d ",&tempbalance );
            fscanf(file,"%s ",tempbranch );
            printf("\n i is %d \n",i);
            /* strcpy( ((acc+i)->name) , tempname);
            (acc+i)->id = tempid;
            (acc+i)->balance = tempbalance;
            strcpy( ((acc+i)->branch) , tempbranch); */
        }
        fclose(file);
    }
    return num;
}

Answer 1

There are a lot of issues with the posted code. It is unclear how a separate allocation function is helpful, and the use of the file-scope variable acc is not advisable. I see no point in using realloc() here, since allocation is done only once. If you do use realloc(), you should store the result in a temporary pointer, because the function can return a NULL pointer if there is an allocation error. This leads to a memory leak if you assign directly to the pointer that you are reallocating from. I have rewritten the code to illustrate some fixes, trying to maintain the general structure of the original code.

You should check the return values of the functions that you call. realloc() and malloc() return a pointer to the allocated memory, or a NULL pointer in the event of an allocation error. You should check this value and handle the result. The scanf() functions return the number of successful assignments made. You should check this value to verify that input is as expected. It is almost always a bad idea to control a loop with feof(), because this function relies upon the end-of-file indicator being set, and this indicator is only set when an I/O operation has failed.

In the program below, fgets() is used to read a line of input from the file into a buffer, and sscanf() is used to extract the input data from the buffer. In the allocation phase, EOF or an empty line signals the end of the data. No parsing is done here, only a count of lines. For each line, space is allocated for an account. Note that the code checks for errors on opening and closing the file, as well as for an allocation error.

The loadfile() function again uses fgets() to read a line of input into buffer, and then uses sscanf() to scan the buffer. Note the use of width specifiers for the strings. These are one less than the size of the arrays that they read into, to leave space for the '\0' placed at the end of the strings by sscanf(). Also note that in the event that fewer than 4 assignments are made, the program exits with an error message. If the assignments to the temporary variables were successful, the account is updated with the data.

There are many ways that this code could be improved (most obviously by getting rid of the global variable acc), but this should provide you with a good starting point.

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

typedef struct {
    char name[20];
    int id;
    int balance;
    char branch[10];
} account;

account *acc = NULL;

int allocate(void)
{
    int num = 0 ;
    char buffer[1000];
    FILE *file;

    file = fopen("D://bank.txt","r");
    if (file == NULL) {
        fprintf(stderr, "Unable to open file in allocate()\n");
        exit(EXIT_FAILURE);
    }

    while (fgets(buffer, sizeof(buffer), file) != NULL &&
           buffer[0] != '\n') {
        num++;
    }

    acc = malloc(num * sizeof(*acc));
    if (acc == NULL) {
        fprintf(stderr, "Allocation error in allocate()\n");
        exit(EXIT_FAILURE);
    }

    if (fclose(file) != 0) {
        fprintf(stderr, "Unable to close file in allocate()\n");
        exit(EXIT_FAILURE);
    }

    printf(" num in allocate function : %d\n",num);
    return num;
}

int loadfile(void)
{
    int num = allocate();
    char buffer[1000], tempname[20],tempbranch[10];
    int tempid ,tempbalance;
    if ( num != 0 ) {
        int i = 0 ;
        FILE *file;

        file = fopen("D://bank.txt","r+");
        if (file == NULL) {
            fprintf(stderr, "Unable to open file in loadfile()\n");
            exit(EXIT_FAILURE);
        }

        while (fgets(buffer, sizeof(buffer), file) != NULL
               && buffer[0] != '\n') {
            if (sscanf(buffer, "%19s %d %d %9s",
                       tempname, &tempid, &tempbalance, tempbranch) != 4) {
                fprintf(stderr, "%d: Malformed input data\n", i);
                exit(EXIT_FAILURE);
            }

            strcpy(acc[i].name, tempname);
            acc[i].id = tempid;
            acc[i].balance = tempbalance;
            strcpy(acc[i].branch, tempbranch);
            ++i;
        }

        if (fclose(file) != 0) {
            fprintf(stderr, "Unable to open file in loadfile()\n");
            exit(EXIT_FAILURE);
        }
    }

    return num;
}

int main(void)
{
    int num = loadfile();
    for (int i = 0; i < num; i++) {
        printf("%s %d %d %s\n",
               acc[i].name, acc[i].id, acc[i].balance, acc[i].branch);
    }

    return 0;
}

Answer 2

I can't explain all your problems. It will take me hours. I hope this code will be self-describing. Ask me in a comment if you need some help.

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

typedef struct {
  char name[20];
  int id;
  int balance;
  char branch[10];
} account_t;

static account_t *parse_account_file(FILE *file, size_t *size) {
  if (file == NULL || size == NULL) {
    return NULL;
  }

  size_t i = 0;
  account_t *account = malloc(sizeof *account);
  if (account == NULL) {
    return NULL;
  }

  int ret;
  while (
      (ret = fscanf(file, "%19s %d %d %9s\n", account[i].name, &account[i].id,
                    &account[i].balance, account[i].branch)) == 4) {
    account_t *old = account;
    account = realloc(account, sizeof *account * (++i + 1));
    if (account == NULL) {
      free(old);
      return NULL;
    }
  }

  if (ret == EOF) {
    if (ferror(file)) {
      perror("parse_account_file()");
    } else {
      *size = i;
      account_t *old = account;
      account = realloc(account, sizeof *account * i);
      if (account == NULL) {
        return old;
      }
      return account;
    }
  } else {
    fprintf(stderr, "error parsing\n");
  }

  free(account);

  return NULL;
}

int main(void) {
  char const *name = "D:\\bank.txt";
  FILE *file = stdin;

  size_t size;
  account_t *account = parse_account_file(file, &size);
  fclose(file);
  if (account == NULL) {
    return 1;
  }

  for (size_t i = 0; i < size; i++) {
    printf("%s %d %d %s\n", account[i].name, account[i].id, account[i].balance,
           account[i].branch);
  }

  free(account);
}