0

I'm trying to read my linked list from file with no luck. Struct is properly saved with fprintf (couldn't get it to work with fwrite). Here is my struct:

typedef struct database
{
    int id;
    int nrindeksu;
    char *imie;
    char *nazwisko;
    char *wydzial;
    char *kierunek;
    struct database* link;
} data;

char buffer[1024]; //changed buffer size

I'm saving linked list to file using this function: Edit: After some changes to other functions this one crashes.

void save(data* head)
{
    FILE *fp;
    fp = fopen("test.txt", "w");
    data* current = head->link;

    while (current != NULL) {
        fprintf(fp,"%d ",current->nrindeksu);
        fprintf(fp,"%s ",current->imie);
        fprintf(fp,"%s ",current->nazwisko);
        fprintf(fp,"%s ",current->wydzial);
        fprintf(fp,"%s\n",current->kierunek);
        current = current->link;
    }
    fclose(fp);
}

and trying to read it from file with this:

void read(data* head)
{
    data* current = head;
    FILE *fp;
    fp = fopen("test.txt", "r");

    while(//I still need help with this loop)
    fscanf(fp, "%d", current->link->nrindeksu);

    fscanf(fp, "%s", buffer);
    current->link->imie=malloc(strlen(buffer) + 1);
    if(current->link->imie == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->imie, buffer);

    fscanf(fp, "%s", buffer);
    current->link->nazwisko=malloc(strlen(buffer) + 1);
    if(current->link->nazwisko == NULL) {
        fprintf(stderr, "Malloc error");
        exit(0);
    }
    strcpy(current->link->nazwisko, buffer);

    fscanf(fp, "%s", buffer);
    current->link->wydzial=malloc(strlen(buffer) + 1);
    if(current->link->wydzial == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->wydzial, buffer);

    fscanf(fp, "%s", buffer);
    current->link->kierunek=malloc(strlen(buffer) + 1);
    if(current->link->kierunek == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->kierunek, buffer);

    fclose(fp);
}

Any idea why it doesn't read anything? Also I don't know how to code a loop that will determine where it should stop reading from file.

Edit: I'm using this function to add items to the list: 

void add_bottom(data* head) 
{
    data* current = head;

    while (current->link != NULL)
    {       
        current = current->link;
    }

    current->link = malloc(sizeof(*current));//changed every malloc 
    if(current->link == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }

    printf("Podaj nr indeksu studenta: ");
    scanf("%d", current->link->nrindeksu);

    printf("Podaj imie studenta: ");
    fflush(stdin);
    scanf("%19[^\n]", buffer);
    current->link->imie=malloc(strlen(buffer) + 1);
    if(current->link->imie == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->imie, buffer);

    printf("Podaj nazwisko studenta: ");
    fflush(stdin);
    scanf("%19[^\n]", buffer);
    current->link->nazwisko=malloc(strlen(buffer) + 1);
    if(current->link->nazwisko == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->nazwisko, buffer);

    printf("Podaj wydzial studenta: ");
    fflush(stdin);
    scanf("%29[^\n]", buffer);
    current->link->wydzial=malloc(strlen(buffer) + 1);
    if(current->link->wydzial == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->wydzial, buffer);

    printf("Podaj kierunek studenta: ");
    fflush(stdin);
    scanf("%29[^\n]", buffer);
    current->link->kierunek=malloc(strlen(buffer) + 1);
    if(current->link->kierunek == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    strcpy(current->link->kierunek, buffer);

    current->link->link = NULL;
}

Main:

int main(int argc, char *argv[]) 
{
    char c;
    head = init(head);
    if(head == NULL) {
        fprintf(stderr, "Malloc error"); 
        exit(0);
    }
    read(head);
    do{
        printf("Baza danych - wybierz opcje: [d]odaj, [w]yswietl, [u]sun, [k]oniec: ");
        scanf("%c", &c);
        if(c=='d')
        {
            system("cls");
            add_bottom(head);
        }
        if(c=='w')
        {
            system("cls");
            show(head);
        }
        if(c=='u')
        {
            system("cls");
            show(head);
            remove_by_id(&head);
            system("cls");
            show(head);
            printf("\n");
        }
        fflush(stdin);

    }while(c!='k');
    save(head);
    free(head);
    return 0;
}

Also function init is just

head = calloc(1,sizeof(data));

This way first node is always NULL, couldn't get it to work other way to create or edit first node.

avery121
  • 3
  • 3
  • Thirty byte buffers are extremely risky. Are you sure you can't spare a bit more memory for those? 1024 is a more sane default, but dynamic length is even better, avoiding buffer overflows. – tadman Jul 14 '17 at 16:20
  • Each of those allocations for your internal strings is one character short. You're not leaving space for the terminator written by `strcpy`. If POSIX is an option, just use [`strdup`](http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html), which will allocate and copy for you. (you still need to `free` when appropriate). – WhozCraig Jul 14 '17 at 16:20
  • Your read function throws away anything previously saved after the first node (by assigning the new node to current->link ,when there might be other nodes already linked). – Jim Lewis Jul 14 '17 at 16:24
  • You have a major problem here: `current->link = malloc(sizeof(data));`. You are allocating a pointer, not the entire structure here since `data` is a `data *`, not a `data`. – Mad Physicist Jul 14 '17 at 16:32
  • Your `read` function appears to be missing a whole bunch of declarations, also, any kind of loop... – Mad Physicist Jul 14 '17 at 16:33
  • The loop should end when `fgets` returns `NULL`: https://stackoverflow.com/q/5670973/2988730 – Mad Physicist Jul 14 '17 at 16:35
  • @MadPhysicist - Can you please explain the comment above where you are saying `current->link = malloc(sizeof(data));` is creating space only for a pointer? Or am I reading it wrong? Clearly, `data` is not a pointer, so the memory created in that statement would be sufficient for a new instance of the entire struct. – ryyker Jul 14 '17 at 17:53
  • When i'm using `current->link = malloc(sizeof(data *));` or `current->link = malloc(sizeof(*current));` it doesn't work. It creates memory for i belive int `nrindeksu` and two chars `imie` and `nazwisko` but then crashes. Works with `current->link = malloc(sizeof(data));` though. – avery121 Jul 14 '17 at 17:58
  • When I test it, I get a compiler warning (_Not enough space for casting expression to 'pointer to struct database_) for `malloc(sizeof(data *));`. However, the statement: `malloc(sizeof(*current));` should yield the same memory as the statement `malloc(sizeof(data));` because `data` is equivalent to `*current`. i.e. the type `data` is used to create `*current`. I tested both versions to verify. – ryyker Jul 14 '17 at 18:03
  • @rykker. The type data is being shadowed by `data* current = head;`. I would put a note about proper naming conventions into your answer. – Mad Physicist Jul 14 '17 at 18:08
  • @MadPhysicist - I am actually finding that the posted code will not even compile as is (eg `fscanf("%d", &nrindeksu);`) I am reluctant to continue at this point, and will wait until OP fixes that issue. – ryyker Jul 14 '17 at 18:16
  • @rykker Already fixed that. Funny thing it actually compiled without errors that's why i didn't see that. Thanks for help. – avery121 Jul 14 '17 at 18:19
  • 1
    Can you edit your post please (with a note in the text body explicitly explaining it has been edited) to reflect the same code you are using. you might also show what the input file you are using looks like, and show how you are calling these functions. (i.e. a small excerpt of `main()` perhaps). Thanks. – ryyker Jul 14 '17 at 18:25
  • I see you've edited your code, Thanks. I created a very simple read and save function based on modifications of your original versions of those functions and got it to work. By the way, _[here is a link to a tutorial](http://www.geeksforgeeks.org/linked-list-set-2-inserting-a-node/)_ that I recommend on creating and using linked lists. The concept of inserting a node, including `push`, `pop` and `insert after` are discussed and illustrated. – ryyker Jul 14 '17 at 19:18
  • Thank you for the accept. I am going to add an example based on the tutorial provided by the link (see comment above) in my answer. It is architected a little different from yours, but the techniques allow for easier reading, and memory is cleaned up better. Please take a look. – ryyker Jul 14 '17 at 21:03

1 Answers1

1

The comments site several issues with your code. For illustration on a few of them...

The statements (6 of them):

fscanf("%d", &nrindeksu);

...as is will not allow the code you posted to compile, as fscanf() requires FILE * to be used as the first argument. 

fscanf(fp, "%d", &nrindeksu);

The function strlen(nazwisko) (as with all other similar lines in your post) returns only the length of the buffer without accounting for the NULL terminator, which will be required of the length of the receiving buffer, current->link->nazwisko .

At the very least, you can address this by adding space for the NULL terminator explicitly:

current->link->nazwisko=(char*)malloc(sizeof(char) * strlen(nazwisko) + 1);
                        ^^^^^^^       ^^^^^^^^^^^^^^                  ^^^
                            not necessary                          necessary

Because sizeof(char) is always == 1, it is not necessary

Cleaned up:

current->link->nazwisko=malloc(strlen(nazwisko) + 1);

Or, as suggested in the comments (if POSIX is an option) you can use strdup()

char *tmp = 0;
tmp = strdup(nazwisko);
if(tmp)
{
     current->link->nazwisko = tmp;
     strcpy(current->link->nazwisko, nazwisko);
}

Another minor point. The following is legal, and does allocate sufficient space: 

current->link = malloc(sizeof(data));

But this form:

current->link = malloc(sizeof(*current));

is generally recommended as it is better that the variable, not the type be used in the sizeof expression.

One additional comment, it is not necessary to cast the return of [m][c]alloc when using C.

EDIT (edits to your code that allow it to compile and run. [complete with memory leaks]) 

//does not include global declarations of struct data and variables
void read(data** head); //so object can be changed when passes as function argument
//             ^
void save(data* head);


int main(void)
{
    data *h = calloc(1, sizeof(data));
    read(&h);  //note, passing to data **, not data *
    save(h);

    return 0;
}

void save(data* head)
{
    FILE *fp;
    fp = fopen("text2.txt", "w");
    //data* current = head->link;

    fprintf(fp,"%d ",head->link->nrindeksu);
    fprintf(fp,"%s ",head->link->imie);
    fprintf(fp,"%s ",head->link->nazwisko);
    fprintf(fp,"%s ",head->link->wydzial);
    fprintf(fp,"%s\n",head->link->kierunek);
    fclose(fp);
}


void read(data **head)
{
    data *current = calloc(1, sizeof(*current));
    current = *head;
    int count;
    FILE *fp;
    fp = fopen(".\\text.txt", "r");

    {
        current->link = malloc(sizeof(data));
        if(current->link == NULL) {
            fprintf(stderr, "Malloc error"); 
            exit(0);
        }
        count = fscanf(fp,"%d %s %s %s %s", &nrindeksu, imie, nazwisko, wydzial, kierunek);
        if(count != 5)
        {
            free(current);
            return 
        }
        current->link->nrindeksu = nrindeksu;

        current->link->imie=(char*)malloc(sizeof(char) * strlen(imie)+1);
        if(current->link->imie == NULL) {
            fprintf(stderr, "Malloc error"); 
            exit(0);
        }
        strcpy(current->link->imie, imie);

        current->link->nazwisko=(char*)malloc(sizeof(char) * strlen(nazwisko)+1);
        if(current->link->nazwisko == NULL) {
            fprintf(stderr, "Malloc error"); 
            exit(0);
        }
        strcpy(current->link->nazwisko, nazwisko);

        current->link->wydzial=(char*)malloc(sizeof(char) * strlen(wydzial)+1);
        if(current->link->wydzial == NULL) {
            fprintf(stderr, "Malloc error"); 
            exit(0);
        }
        strcpy(current->link->wydzial, wydzial);

        current->link->kierunek=(char*)malloc(sizeof(char) * strlen(kierunek)+1);
        if(current->link->kierunek == NULL) {
            fprintf(stderr, "Malloc error");         
            exit(0);
        }
        strcpy(current->link->kierunek, kierunek);

        current->link->link = NULL; 
    }
    fclose(fp);
}

Note: the above code does read the data into a node, and is able write the node data into a file, but you will have to do the memory clean up. Nothing is freed. The link I posted in comments above will help to address that.

EDIT 2 (based on functions in this tutorial link)

The following will read text file (see example file at bottom of code) into a linked list, then write the contents of the list into a second text file, then clean up all the memory used.

typedef struct database
{
    int id;
    int nrindeksu;
    char *imie;
    char *nazwisko;
    char *wydzial;
    char *kierunek;
    struct database* link;
}data;

typedef struct {
    int id;
    int nrindeksu;
    char imie[20];
    char nazwisko[20];
    char wydzial[30];
    char kierunek[30];
}LINE;

LINE line;

void push(data** head_ref, LINE *line);
void store(data *d, FILE *p);
void cleanup(data *d);

int main(void)
{
    data *head = NULL;
    FILE *fp;
    fp = fopen(".\\text1.txt", "r");
    if(fp)
    {
        while(fscanf(fp,"%d %s %s %s %s", &line.nrindeksu, line.imie, line.nazwisko, line.wydzial, line.kierunek)== 5)
        {
            push(&head, &line); 
        }
        fclose(fp);
    }
    fp = fopen(".\\text2.txt", "w");
    if(fp)
    {
        store(head, fp);
        fclose(fp);
    }
    cleanup(head);
    return 0;
}

/// synonymous with your "read()" 
void push(data** head_ref, LINE *line)
{
    /* 1. allocate node */
    data *new_node = malloc(sizeof(*new_node));

    /* 2. put in the data  */
    new_node->id  = line->id;
    new_node->nrindeksu  = line->nrindeksu;
    new_node->imie = calloc(1, strlen(line->imie)+1);
    strcpy(new_node->imie, line->imie); 
    new_node->nazwisko = calloc(1, strlen(line->nazwisko)+1);
    strcpy(new_node->nazwisko, line->nazwisko); 
    new_node->wydzial = calloc(1, strlen(line->wydzial)+1);
    strcpy(new_node->wydzial, line->wydzial); 
    new_node->kierunek = calloc(1, strlen(line->kierunek)+1);
    strcpy(new_node->kierunek, line->kierunek); 

    /* 3. Make next of new node as head */
    new_node->link = (*head_ref);

    /* 4. move the head to point to the new node */
    (*head_ref)    = new_node;
}


void store(data *d, FILE *p)
{
    char line[260];
    while(d != NULL)
    {
        sprintf(line, "%d %s %s %s %s\n", d->nrindeksu, d->imie, d->nazwisko, d->wydzial, d->kierunek);
        fputs(line, p);
        d = d->link;
    }
}

void cleanup(data *d)
{
    data *tmp;

    while(d != NULL)
    {
        tmp = d;

        if(tmp->imie)   free(d->imie);
        if(tmp->nazwisko) free(d->nazwisko);
        if(tmp->wydzial)    free(d->wydzial);
        if(tmp->kierunek) free(d->kierunek);
        d = d->link;
        free(tmp);
    }
    free(d);
}

Text file referenced as input for code sample:

text1.txt:

0 string01 string02 string03 string04
1 string11 string12 string13 string14
2 string21 string22 string23 string24
3 string31 string32 string33 strin3g4
4 string41 string42 string43 string44
5 string51 string52 string53 string54
6 string61 string62 string63 string64
7 string71 string72 string73 string74
8 string81 string82 string83 string84
9 string91 string92 string93 string94
ryyker
  • 22,849
  • 3
  • 43
  • 87