The way you have factored your code (1) to handle the list; and (2) to add data to the list, it so confused, and so lacking in validation that it is no wonder you are having difficulty sorting it out.
The read of the data is flawed from the beginning. See Why is while ( !feof (file) ) always wrong?. Further, you fail to validate a single return of fscanf. If one read fails, you invoke Undefined Behavior by blindly using the indeterminate value (and likely every value from that point forward will be indeterminate). All bets are over at that point.
However, you are to be commended for your use of #define to define constants you need, but you then fail to protect your array bounds by including field-width modifiers with all char* conversion specifiers. While you #define constants, you then turn around and hard-code your file name. Don't do that. Pass your filename as an argument to your program or prompt for its entry.
Whenever processing a "line-of-data" at a time, you should use a line-oriented input function such as fgets or POSIX getline and then parse the values you need from the line of data. This provides the benefit of allowing separate validations of the (1) read of the data from the file; and (2) the parse of the values from the resulting buffer. If for some reason there is an error in the format, your parse will fail, and you can simply continue the read loop and read the next line -- without the risk of Undefined Behavior.
When you are creating a list, all you need is a single append() function which will create the list if it does not exist, and allocate and add each additional node to the list as needed. Your code looks to attempt a simple forward-chaining for adding nodes to your list (which is fine, but without more will result in the list being held in memory in reverse order)
Don't mix reading data with list operations. Instead read and pass the data required to your append() function. While this largely up to you, failure to separate your read/parse and append will just lead to list functions at are not reusable.
For example, instead of trying to read and parse data within your list function, open your file in main() and parse the data to a temporary goodsinfo1 struct and pass the list address and pointer to your temporary data to your append function. You could do something similar to the following for your read and parse of data and passing the needed values to your function:
int main (int argc, char **argv)
{
char buf[MAXC]; /* read buffer */
size_t linecnt = 0; /* line counter */
goodslist *list = NULL; /* linked list pointer */
FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;
if (!fp) { /* validate file open for reading */
perror ("fopen-file");
return 1;
}
while (fgets (buf, MAXC, fp)) { /* read each line of data */
goodsinfo tmp = { .goods_id = "" }; /* temp data struct */
/* parse and validate data in buf (can be separate function) */
if (sscanf (buf, "%29s %29s %d %29s %d %d", tmp.goods_id,
tmp.goods_name, &tmp.goods_price, tmp.goods_discount,
&tmp.goods_amount, &tmp.goods_remain) != 6) {
fprintf (stderr, "error: invalid format line %zu.\n", linecnt+1);
continue;
}
if (!append (&list, &tmp)) /* append to list/validate */
break;
}
if (fp != stdin) /* close file if not stding */
fclose (fp);
prn_list (list); /* print list */
free_list (list); /* free list data */
return 0;
}
(note: the program takes the filename to read data from as the first argument, or reads from stdin by default if no filename is provided. Also note, you declare your list as a pointer to goodslist, not a pointer-to-pointer-to goodslist)
With your data read and parsed, your append() function simply has to allocate storage for data and allocate storage for a new list node. It only has two cases to handle (1) is the list empty? -- leave node->next = NULL; otherwise (2) set node->next equal to the current list address before assigning the address for your new node as the new list address to chain your nodes together, e.g.
/* function to allocate goodslist node and append allocated goodsinfo
* data to list. Takes address of list pointer and pointer to goodsinfo data
* to append to list. Returns pointer new node on success, NULL otherwise.
*/
goodsinfo *append (goodslist **l, goodsinfo *tmp)
{
goodsinfo *data = malloc (sizeof *data); /* allocate/validate data */
if (!data) {
perror ("malloc-data");
return NULL;
}
*data = *tmp; /* fill allocated data block with tmp values */
/* allocate/validate list node */
goodslist *node = malloc (sizeof *node);
if (!node) {
perror ("malloc-node");
free (data);
return NULL;
}
node->data = data; /* initialize data and set next NULL */
node->next = NULL;
if (*l) /* if list exists, chain next to list */
node->next = *l;
return ((*l = node)->data); /* assign new node as list, return data */
}
Putting it altogether you could do something like the following:
#include <stdio.h>
#include <stdlib.h>
#define MAX_ID_LEN 30
#define MAX_NAME_LEN MAX_ID_LEN
#define MAX_PRICE_LEN MAX_NAME_LEN
#define MAX_DISCOUNT_LEN MAX_PRICE_LEN
#define MAXC 1024 /* read buffer size (don't skimp) */
typedef struct {
char goods_id[MAX_ID_LEN];
char goods_name[MAX_NAME_LEN];
int goods_price;
char goods_discount[MAX_DISCOUNT_LEN];
int goods_amount;
int goods_remain;
} goodsinfo;
typedef struct goodslist {
goodsinfo *data; /* make data a pointer and allocate */
struct goodslist *next;
} goodslist;
/* bool check_nullfile(void)
* (poor test, if first char not 0-9, test fails)
*/
/* function to allocate goodslist node and append allocated goodsinfo
* data to list. Takes address of list pointer and pointer to goodsinfo data
* to append to list. Returns pointer new node on success, NULL otherwise.
*/
goodsinfo *append (goodslist **l, goodsinfo *tmp)
{
goodsinfo *data = malloc (sizeof *data); /* allocate/validate data */
if (!data) {
perror ("malloc-data");
return NULL;
}
*data = *tmp; /* fill allocated data block with tmp values */
/* allocate/validate list node */
goodslist *node = malloc (sizeof *node);
if (!node) {
perror ("malloc-node");
free (data);
return NULL;
}
node->data = data; /* initialize data and set next NULL */
node->next = NULL;
if (*l) /* if list exists, chain next to list */
node->next = *l;
return ((*l = node)->data); /* assign new node as list, return data */
}
/* simple print list function */
void prn_list (goodslist *l)
{
if (!l)
return;
while (l) {
printf (" %-8s %-8s %8d %-8s %8d %9d\n", l->data->goods_id,
l->data->goods_name, l->data->goods_price,
l->data->goods_discount, l->data->goods_amount,
l->data->goods_remain);
l = l->next;
}
}
/* simple free list function */
void free_list (goodslist *l)
{
if (!l)
return;
goodslist *iter = l;
while (iter) {
goodslist *victim = iter;
free (iter->data);
iter = iter->next;
free (victim);
}
}
int main (int argc, char **argv)
{
char buf[MAXC]; /* read buffer */
size_t linecnt = 0; /* line counter */
goodslist *list = NULL; /* linked list pointer */
FILE *fp = argc > 1 ? fopen (argv[1], "r") : stdin;
if (!fp) { /* validate file open for reading */
perror ("fopen-file");
return 1;
}
while (fgets (buf, MAXC, fp)) { /* read each line of data */
goodsinfo tmp = { .goods_id = "" }; /* temp data struct */
/* parse and validate data in buf (can be separate function) */
if (sscanf (buf, "%29s %29s %d %29s %d %d", tmp.goods_id,
tmp.goods_name, &tmp.goods_price, tmp.goods_discount,
&tmp.goods_amount, &tmp.goods_remain) != 6) {
fprintf (stderr, "error: invalid format line %zu.\n", linecnt+1);
continue;
}
if (!append (&list, &tmp)) /* append to list/validate */
break;
}
if (fp != stdin) /* close file if not stding */
fclose (fp);
prn_list (list); /* print list */
free_list (list); /* free list data */
return 0;
}
(note: your bool check_nullfile(void) does more harm than good and will fail if the first non-whitespace character isn't a digit)
Example Use/Output
(note: when using chaining without keeping a "last" pointer results in the list nodes stored in reverse order)
$ ./bin/ll_goodslist dat/goodsinfo.txt
1004 new5 100 0.8 70 80
1003 new4 88 0.8 70 80
1002 new3 70 0.8 10 10
1001 new2 80 0.9 80 80
1000 new1 90 0.9 90 80
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 insure 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/ll_goodslist dat/goodsinfo.txt
==3493== Memcheck, a memory error detector
==3493== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==3493== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==3493== Command: ./bin/ll_goodslist dat/goodsinfo.txt
==3493==
1004 new5 100 0.8 70 80
1003 new4 88 0.8 70 80
1002 new3 70 0.8 10 10
1001 new2 80 0.9 80 80
1000 new1 90 0.9 90 80
==3493==
==3493== HEAP SUMMARY:
==3493== in use at exit: 0 bytes in 0 blocks
==3493== total heap usage: 11 allocs, 11 frees, 1,152 bytes allocated
==3493==
==3493== All heap blocks were freed -- no leaks are possible
==3493==
==3493== For counts of detected and suppressed errors, rerun with: -v
==3493== 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 further questions.
footnotes
- While not an error, C generally avoids the use of
camelCase or MixedCase variable names in favor of all lower-case while reserving upper-case names for use with macros and constants. It is a matter of style -- so it is completely up to you, but failing to follow it can lead to the wrong first impression in some circles.
