0

Here's my code. I keep getting errors. I think the part where the error occurs is the part where the int score[3] is read. How can I solve this?

// code

typedef struct {
    int no;
    char name[TSIZE];
    int score[3];
    int sum;
    double avg;
} Score;

int num;
    if (fscanf(file, "%d%*c", &num) != 1){
        printf("$ !ERROR: Wrong file format to num.\n");
        exit(1);
    }

    *ptr_s_list = (Score *)malloc(sizeof(Score) * num);
    if (*ptr_s_list == NULL){
        printf("$ !ERROR: Malloc failed.\n");
        exit(1);
    }

    for (int s = 0; s < num; ++s){
        if (fscanf(file, "%d%*c", &(*ptr_s_list)[*ptr_s_items].no) != 1
        || fscanf(file, "%[^\n]%*c", (*ptr_s_list)[*ptr_s_items].name) != 1
        || fscanf(file, "%d %d %d", &(*ptr_s_list)[*ptr_s_items].score[0], &(*ptr_s_list)[*ptr_s_items].score[1], &(*ptr_s_list)[*ptr_s_items].score[2]) != 1
        || fscanf(file, "%d%*c", &(*ptr_s_list)[*ptr_s_items].sum) != 1
        || fscanf(file, "%lf%*c", &(*ptr_s_list)[*ptr_s_items].avg) != 1
        ){    
            printf("$ !ERROR: Wrong file format to \'%s\'.\n", filename);
            exit(1);
        }
        (*ptr_s_items)++;
    }
// txt
3 // this int num 
1   jay 100 90  80  270 90.0
2   key 90  80  65  235 78.3
3   ray 90  45  65  200 66.7

Tell me how to solve it.

  • 2
    Read the doco for `scanf()`... `%[^\n]` will try to read the entire rest of the line (to the LF character.) And, `"%d %d %d"` is not **1** item. And, you don't have to _discard_ the intervening whitespace; `scanf()` does this for you. Why not try reading one thing and getting that to work before attempting all of these different things? – Fe2O3 Mar 31 '23 at 04:30
  • 1
    Also, post a program instead of an incomplete snippet. Implement better error handling so you don't have to guess which of the 5 `fscanf()` are failing. Why are you not reading a line at a time? Is the `//` in your input file? Your code doesn't handle it. – Allan Wind Mar 31 '23 at 04:31
  • Always use a maximum field with when reading a sting, otherwise you have no protection against buffer overflows. Don't cast the `void *` from `malloc()`, – Allan Wind Mar 31 '23 at 04:37
  • `I think the part where the error occurs` - I suggest you look into working with a [debugger](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems). This helps you pinpoint the problem precisely. – Gereon Mar 31 '23 at 07:10

2 Answers2

1

Here is a implementation that parses the sample.txt input file (with the // stuff removed):

3
1   jay 100 90  80  270 90.0
2   key 90  80  65  235 78.3
3   ray 90  45  65  200 66.7
  1. Add missing headers
  2. Define TSIZE; notice implementation of the struct changed to do TSIZE+1 to use the maximum field width when reading a string.
  3. Code should be in a function so used main() here.
  4. Use size_t instead of int for num to avoid input of, say, -1. Till need to check that it's non-zero as behavior of malloc(0) is implementation defined.
  5. Used a single fscanf() to read each line. There are 7 fields, @Fe2O3 already pointed out the incorrect return value check of 1 with 3 fields being read. Use a maximum field width when reading the name string.
  6. Consider reading a line with fgets() and process it with sscanf(). It gives you a natural synchronization point, say, by skipping the faulty line. It also gives you access to the invalid data to generate a better error message.
#include <stdio.h>
#include <stdlib.h>

#define TSIZE 42
#define str(s) str2(s)
#define str2(s) #s

typedef struct {
    int no;
    char name[TSIZE+1];
    int score[3];
    int sum;
    double avg;
} Score;

int main() {
    // declarations with well-defined values needed before first goto
    FILE *f;
    Score *ptr_s_list = NULL;

    f = fopen("sample.txt", "r");
    if(!f) {
        printf("could not open filed\n");
        goto out;
    }

    size_t num;
    if(fscanf(f, "%zu", &num) != 1 || !num) {
        printf("$ !ERROR: Wrong file format to num.\n");
        goto out;
    }
    ptr_s_list = malloc(sizeof *ptr_s_list * num);
    if(!ptr_s_list) {
        printf("malloc failed\n");
        goto out;
    }
    for(Score *p = ptr_s_list; p < ptr_s_list + num; p++) {
        int rv = fscanf(f, "%d%" str(TSIZE) "s%d%d%d%d%lf",
            &p->no,
            p->name,
            &p->score[0],
            &p->score[1],
            &p->score[2],
            &p->sum,
            &p->avg
        );
        if(rv != 7) {
            printf("$ !ERROR: Wrong file format; %d of 7 fields read\n", rv);
            goto out;
        }
        // printf("%d %s %d %d %d %d %g\n", p->no, p->name, p->score[0], p->score[1], p->score[2], p->sum, p->av  g);
    }

out:
    free(ptr_s_list);
    if(f) fclose(f);
}
Allan Wind
  • 23,068
  • 5
  • 28
  • 38
1

You have a number of issues, not the least of which is the lack of a Minimal Complete Reproducible Example, but your primary problem is you are thinking about reading and parsing the information from the file in an overly complicated and very fragile way.

When you look at your input file, e.g.

cat dat/txtscore.txt
// txt
3 // this int num
1   jay 100 90  80  270 90.0
2   key 90  80  65  235 78.3
3   ray 90  45  65  200 66.7

(empty line at end intentional)

You don't care about the 3 telling you how many records there are -- really. You don't care about the comment lines or the empty line at the end. All you care about are the lines holding the data you want. To that end, all you really care about are the lines that will parse successfully given your input routine.

No matter what you are reading, a good general approach is to read an entire line of data into a buffer (temporary character array) with fgets() and then attempt to parse the line with sscanf(). If the parse with sscanf() fails to read the data you want -- ignore the line and get the next, e.g.

#define TSIZE 64
...

score *readfile (FILE *fp, score **ptr_s_list, size_t *nelem)
{
  ...
  char buf[TSIZE * TSIZE];      /* buffer to hold each line */
  ...
  /* read a line of input at a time into buf */
  while (fgets (buf, sizeof buf, fp)) {
    score s;                    /* temporary stuct to fill */
    /* parse record using sscanf from buf, ALWAYS use field-width
     * modifier when read into an array to protect array bounds.
     */
    if (sscanf (buf, "%d %63s %d %d %d %d %lf", &s.no, s.name,
                &s.score[0], &s.score[1], &s.score[2], 
                &s.sum, &s.avg) != 7) {
      continue;   /* if parse fails, ignore line, get next */
    }
    ...

This greatly simplifies reading the file and will make your code far more robust.

Handling Allocation/Reallocation

To begin, in C, there is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc?. While you are reading and parsing lines of data from your file, you could reallocate once per-line, but that is a bit expensive (less so with a mmap backed malloc, but still relatively expensive). So instead, simply keep two counters, (1) holding the number of structs used and (2) a second holding the number of structs allocated and available avail. You only need to reallocate when used == avail.

When you reallocate, you always realloc() using a temporary pointer. Why? When (not if) you run out of memory and realloc() fails, it will return NULL. If you are reallocating using your pointer, e.g. *ptr_s_list = realloc (*ptr_s_list, ...) you have just overwritten your pointer address with NULL preventing you from being able to free that memory creating a memory-leak.

When realloc() fails it doesn't free() the original block -- meaning your data is still good. If you overwrite your pointer with NULL you have also lost your data. Lesson: "Always reallocate to a temporary pointer".

With that (and reading between the lines looking at your *ptr_s_list and presuming that is due to you passing the address-of the ptr_s_list pointer to your read function), then when reading, on a successful parse, you can simply check if a reallocation is needed before assigning your temporary struct s you filled to your allocated list, e.g.

score *readfile (FILE *fp, score **ptr_s_list, size_t *nelem)
{
  size_t  used = *nelem,        /* element counter */
          avail = *nelem;       /* allocated elements available */
  ...
  void *tmp = NULL;             /* temp pointer to realloc with */

  /* read a line of input at a time into buf */
  while (fgets (buf, sizeof buf, fp)) {
    ...
    /* test if realloc needed */
    if (used == avail) {
      if (!avail) {             /* if first allocation, set to 1 */
        avail = 1;
      }
      /* always realloc to a temporary pointer, below doubles size */
      tmp = realloc (*ptr_s_list, 2 * avail * sizeof **ptr_s_list);
      /* validate EVERY allocation */
      if (!tmp) { /* handle error */
        perror ("realloc-*ptr_s_list-readfile()");
        if (!used) {            /* if no records, return NULL */
          return NULL;
        }
        *nelem = used;          /* update number of elements */
        return *ptr_s_list;     /* return pointer to 1st stuct */
      } 
      avail *= 2;               /* update available stuct count */
      *ptr_s_list = tmp;        /* assign new block to ptr */
    }
    
    (*ptr_s_list)[used++] = s;  /* assign temp struct to element */
  }

Above, note how realloc() is used for the first and all subsequent reallocations. If your original pointer is NULL, then realloc() behaves like malloc() allowing you to simplify your allocation scheme. Also note, if allocating the first struct, avail is set to 1 and from then on, the number of available structs is simply doubled when more are needed. You can grow your allocation by whatever method you like, but doubling is a fair balance between reallocation frequency and allocation growth -- up to you.

Above, by passing a pointer to the number of elements and reallocating from there on, you can also call readfile() multiple time perhaps passing different files to be read while accumulating all of the data in your allocated block of structs. This isn't mandatory, but provides flexibility in how you can use the readfile() function. By returning the pointer to the allocated block of struct, it also allows you to assign different reads to different pointers if you needed to keep track of separate groups of scores. Again, not mandatory, but simply providing flexibility in how it can be used.

Lastly, and optional, before the readfile() function returns it calls realloc() a final time to shrink the allocation to just the number of struct used. (the compiler isn't guaranteed to honor the reduction, but most will) You can do that, again using a temporary pointer, as:

  /* optional - reduce size to used structs */
  tmp = realloc (*ptr_s_list, used * sizeof **ptr_s_list);
  if (!tmp) {   /* handle error - warn */
    perror ("realloc-*ptr_s_list-sizetofit-readfile()");
  }
  else {
    *ptr_s_list = tmp;        /* assign new block to ptr */
  }

Adding a short print function to output all stored records and one way to implement reading and storing all data from your file would be:

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

#define TSIZE 64

typedef struct {
    int no;
    char name[TSIZE];
    int score[3];
    int sum;
    double avg;
} score;

/* funtion to read all scores from file pointer, reallocating
 * *ptr_s_list as needed, updating the pointer nelem with the
 * number of elements read, retuning a pointer to the allocated
 * block of score on success, NULL if no records stored.
 */
score *readfile (FILE *fp, score **ptr_s_list, size_t *nelem)
{
  size_t  used = *nelem,        /* element counter */
          avail = *nelem;       /* allocated elements available */
  
  char buf[TSIZE * TSIZE];      /* buffer to hold each line */
  void *tmp = NULL;             /* temp pointer to realloc with */
  
  /* read a line of input at a time into buf */
  while (fgets (buf, sizeof buf, fp)) {
    score s;                    /* temporary stuct to fill */
    /* parse record using sscanf from buf, ALWAYS use field-width
     * modifier when read into an array to protect array bounds.
     */
    if (sscanf (buf, "%d %63s %d %d %d %d %lf", &s.no, s.name,
                &s.score[0], &s.score[1], &s.score[2], 
                &s.sum, &s.avg) != 7) {
      continue;   /* if parse fails, ignore line, get next */
    }
    /* test if realloc needed */
    if (used == avail) {
      if (!avail) {             /* if first allocation, set to 1 */
        avail = 1;
      }
      /* always realloc to a temporary pointer, below doubles size */
      tmp = realloc (*ptr_s_list, 2 * avail * sizeof **ptr_s_list);
      /* validate EVERY allocation */
      if (!tmp) { /* handle error */
        perror ("realloc-*ptr_s_list-readfile()");
        if (!used) {            /* if no records, return NULL */
          return NULL;
        }
        *nelem = used;          /* update number of elements */
        return *ptr_s_list;     /* return pointer to 1st stuct */
      } 
      avail *= 2;               /* update available stuct count */
      *ptr_s_list = tmp;        /* assign new block to ptr */
    }
    
    (*ptr_s_list)[used++] = s;  /* assign temp struct to element */
  }
  *nelem = used;                /* update element count */
  
  /* optional - reduce size to used structs */
  tmp = realloc (*ptr_s_list, used * sizeof **ptr_s_list);
  if (!tmp) {   /* handle error - warn */
    perror ("realloc-*ptr_s_list-sizetofit-readfile()");
  }
  else {
    *ptr_s_list = tmp;        /* assign new block to ptr */
  }
  
  return *ptr_s_list;         /* return pointer to 1st stuct */
}

/* simple print nelem score records */
void prn_records (score *s, size_t nelem)
{
  printf ("\n%zu records:\n\n", nelem);
  
  for (size_t i = 0; i < nelem; i++) {
    printf ("%3d  %-12s  %3d  %3d  %3d  %4d  %.1f\n", s[i].no, s[i].name,
            s[i].score[0], s[i].score[1], s[i].score[2],
            s[i].sum, s[i].avg);
  }
}

int main (int argc, char **argv) {
  
  size_t nelem = 0;   /* number of elements in allocated block of score */
  score *s = NULL;    /* pointer to allocated block of score */
  /* use filename provided as 1st argument (stdin by default) */
  FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;

  if (!fp) {  /* validate file open for reading */
    perror ("file open failed");
    return 1;
  }
  
  /* read all score records from file, validating at least 1 record read */
  if (!readfile (fp, &s, &nelem)) {
    fputs ("no records read.\n", stderr);
    return 1;
  }
  
  prn_records (s, nelem);   /* print all stored score records */
  free (s);                 /* free allocated memory */
  
  if (fp != stdin) {        /* close file if not stdin */
    fclose (fp);
  }
}

(note: the program expects the filename of the file to read as the first argument on the command-line, or it will read from stdin by default if no argument is provided)

Example Use/Output

With the sample data in the file dat/txtscore.txt as shown above and the program compiled to readtxtscore in my ./bin subdirectory, you would get:

$ ./bin/readtxtscore dat/txtscore.txt

3 records:

  1  jay           100   90   80   270  90.0
  2  key            90   80   65   235  78.3
  3  ray            90   45   65   200  66.7

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to ensure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/readtxtscore dat/txtscore.txt
==11434== Memcheck, a memory error detector
==11434== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11434== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==11434== Command: ./bin/readtxtscore dat/txtscore.txt
==11434==

3 records:

  1  jay           100   90   80   270  90.0
  2  key            90   80   65   235  78.3
  3  ray            90   45   65   200  66.7
==11434==
==11434== HEAP SUMMARY:
==11434==     in use at exit: 0 bytes in 0 blocks
==11434==   total heap usage: 6 allocs, 6 frees, 6,456 bytes allocated
==11434==
==11434== All heap blocks were freed -- no leaks are possible
==11434==
==11434== For lists of detected and suppressed errors, rerun with: -s
==11434== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Look things over and let me know if you have questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85