Help regarding Bus Errors

Hi, it's my first post. I'm a computer engineering student from Italy, and I'm currently studying C.
I'm preparing for my exam and often get problems regarding lists and bus errors. The function is supposed to load data from a text file into an array of list. It gives me a bus error and no futher explanation.

Code:

---

void inserisciDati(P_NodoLista vettore[32]){
        char nomefile[30];
        int day;
        P_NodoLista pcorr, p;

       
        printf("\nInserisci nome file: ");
        gets(nomefile);
       
        FILE *fp;
        if ((fp=fopen(nomefile,"r"))==NULL)
        exit(1);
       
        do{
        P_Misura aus = (P_Misura)malloc(sizeof(Misura));
        fscanf(fp, "%d %s %d %d\n", aus->giorno, aus->citta, aus->tmax, aus->tmin);
                        day = aus->giorno;
                       
                       
                        p = (P_NodoLista)malloc(sizeof(NodoLista));
                        pcorr = vettore[day];
                       
                        while(pcorr!=NULL) {
                                pcorr = pcorr->next;
                        }
                       
                        pcorr->next = p;
                       
                        p->info.giorno;
                        p->info.citta;
                        p->info.tmax;
                        p->info.tmin;
                       
                        p->next = NULL;
                       
        } while (!feof(fp));

}

---

If it can be usefull the struct I use is:

Code:

---

typedef struct misura {
        int giorno;
        char citta[20];
        int tmax;
        int tmin;
} Misura, *P_Misura;

typedef struct nodolista {

        Misura info;
        struct nodolista* next;
} NodoLista, *P_NodoLista;

---

I have got often this error when dealing with lists, so I believe the must be something I constantly do wrong. Thank you for the attention, greetings from Italy :)

Ps: tell me if it can help translating the variables names to English.

Quote:

---

Originally Posted by icepie

Code:

---

                        while(pcorr!=NULL) {
                                pcorr = pcorr->next;
                        }
                       
                        pcorr->next = p;

---

---

Look at the while loop above. What do we know when the while loop ends? That pcorr is NULL. So the next line is NULL->next = p, and we know what happens when we follow the NULL pointer.

If you want to stop one before the end, you need to loop like

Code:

---

while(pcorr->next != NULL)

---

Thank you for the reply.

vettore[day] is an array of 31 pointers to a "struct nodoLista" corresponding to the days of the month, previoulsy all assigned to NULL. I tried to change but still get a bus error.
I first want to check if a list has been initialized for the correspondent day. If not it creates the first element of the list or otherwise it look for the last.

Code:

---

p = (P_NodoLista)malloc(sizeof(NodoLista));
                                               
                        if(vettore[day] == NULL)
                                vettore[day] = p;
                               
                        else{
                               
                                pcorr = vettore[day];
                                while(pcorr != NULL) {
                                        pcorr = pcorr->next;
                                       
                                        printf("\nCiao!");
                                }
                                pcorr->next = p;
                        }
                       
                       
                       
                        p->info.giorno;
                        p->info.citta;
                        p->info.tmax;
                        p->info.tmin;
                       
                        p->next = NULL;

---

I tried changing the data file to read and discovered: applying what you suggested (for which I thank you) would bring me to an infinte while-loop if there were more records for the same day. As it is now it does not fall into an infinite loop, bus halts on a bus error regardless if one or more records for a certain day.

Please excuse me, I realized now what you meant with,

Code:

---

while(pcorr->next != NULL)

---

you were write, that was an error but it still didn't solve the problem

What are these lines supposed to do?

Code:

---

p->info.giorno;
p->info.citta;
p->info.tmax;
p->info.tmin;

---

If you have the above code with the fix in the while loop, I don't see any out-of-bounds accesses (in this part of the code, at least).

In the code above there are several problems

Code:

---

do{
        P_Misura aus = (P_Misura)malloc(sizeof(Misura));
        fscanf(fp, "%d %s %d %d\n", aus->giorno, aus->citta, aus->tmax, aus->tmin);
                        day = aus->giorno;
                       
                       
                        p = (P_NodoLista)malloc(sizeof(NodoLista));
                        pcorr = vettore[day];
                       
                        while(pcorr!=NULL) {
                                pcorr = pcorr->next;
                        }
                       
                        pcorr->next = p;
                       
                        p->info.giorno;
                        p->info.citta;
                        p->info.tmax;
                        p->info.tmin;
                       
                        p->next = NULL;
                       
        } while (!feof(fp));

---

1. do not use feof to control loop - see FAQ - check the return value of fscanf instead
2. aus is leaked memory
3. you do not assign valus to p->info
4. malloc should not be casted in C - see FAQ
5. fscanf should receive pointers to integers - this is reason for crash

They copy the values from the temporary record to the actual node info.

Code:

---

               
p->info.giorno = aus->giorno;
strcpy(p->info.citta, aus->citta);
p->info.tmax = aus->tmax;
p->info.tmin = aus->tmin;

---

I have inserted several printfs at differents points of the code to see where it freezes, and its here:

Code:

---

       
FILE *fp;
if ((fp=fopen(nomefile,"r"))==NULL)
exit(1);

---

Tried to remove the if and the exit, or specifying the the file name directly into the fopen function.

there is nothing wrong with if here
as I said above you should allocate aus on stack to avoid memory leaks
and fix your scanf function

also it is highly recomended to replace the gets function as described in FAQ

Quote:

---

1. do not use feof to control loop - see FAQ - check the return value of fscanf instead
2. aus is leaked memory
3. you do not assign valus to p->info
4. malloc should not be casted in C - see FAQ
5. fscanf should receive pointers to integers - this is reason for crash

---

Thank you vart I managed to solve the problem:

1. Done
2. At the end of the function I added free(aus);
3. Fixed
4. I'll read the FAQ, but this is how my professor prefers to use malloc (I'll stick to it untill the exam)
5. Fixed, and I think it was this that fixed the problem

Thanks again

instead of allocating and deallocating aus you can just use local temp var allocated on stack

Quote:

---

Originally Posted by vart

instead of allocating and deallocating aus you can just use local temp var allocated on stack

---

Probably I'm confused because of the language. The portion of code pasted is inside a function which is called in the main only once, so when it finishes it should automatically free the allocated memory. So free(aus) is useless?

Anyway I have fixed everything up, and moved the allocation on aus outside the do-while. Probably if you look at the code it's easier since my English is not perfect and I'm not familiar with some terms you use:)

Code:

---

void inserisciDati(P_NodoLista vettore[32]){
        char nomefile[30];
        int day;
        P_NodoLista pcorr, p;

        FILE *fp;
        fp=fopen("misure.txt","r");
        P_Misura aus = (P_Misura)malloc(sizeof(Misura));

        while(fscanf(fp, "%d %s %d %d\n", &aus->giorno, aus->citta, &aus->tmax, &aus->tmin)!=EOF){
                        day = aus->giorno;
                        p = (P_NodoLista)malloc(sizeof(NodoLista));

                        if(vettore[day] == NULL){
                                vettore[day] = p;
                        }       
                        else{
                                pcorr = vettore[day];
                                        while(pcorr->next != NULL) {
                                                pcorr = pcorr->next;
                                        }
                                pcorr->next = p;
                        }
                       
                        p->info.giorno = aus->giorno;
                        strcpy(p->info.citta, aus->citta);
                        p->info.tmax = aus->tmax;
                        p->info.tmin = aus->tmin;
                        p->next = NULL;
        } /* while end */
        free(aus);
        fclose(fp);
        printf("\nFunction worked\n");
}

---

Is it C99?
In C89 you cannot mix declarations and other statements

What I mean is

Code:

---

Misura Aus;
while(fscanf(fp, "%d %s %d %d", &Aus.giorno, Aus.citta, &Aus.tmax, &Aus.tmin) ==4)
{
}

---

in this case you do not need to malloc/free aus

and I think better to check that fscanf parsed all 4 formats - otherwise you are still processing wrong data

after the loop is finished you can check feof to be sure the whole file was read or some error occured

Also note that you can copy the struct like this:

Code:

---

p->info = Aus;

---

PS. And I do not understand why days array has 32 members - a longest month I know has only 31 day in it. Or is it for something else?

Yes it's ANSI C, so C99

The control on fscanf you suggest is very helpfull, I'll be using that from now on.
By using the the feof at the end of loop you mean something like this?

Code:

---

while(fscanf(fp, "%d %s %d %d", &Aus.giorno, Aus.citta, &Aus.tmax, &Aus.tmin) ==4)
{

if(feof(fp)!=0) break;
}

---

I was told its not good to use break, because of structured programming rules. Would it be better to add to the while condition, even if it makes it unreadable?

Code:

---

while((fscanf(fp, "%d %s %d %d", &Aus.giorno, Aus.citta, &Aus.tmax, &Aus.tmin) ==4) && feof(fp)!=0)
{
}

---

Regarding the days of the month, in a couple of functions I had written earlier I had forgotten to add -1 each time I needed to access the array from a keyboard selection, so to be quick i just modified the month array. So that "int month[32]", creates from month[0] to month[31].
I have a feeling I'm supposed to feel embarrassed for wasting memory like that. I'll change that later.

Thanks again for the time and help :)

no, if you entered while - read was successful and feof flag will not be set - so no use to checj it

after failed read - when you leave the loop you can check if the flag was set to report succesful parsing of the whole file or not

Code:

---

while(fscanf() == 4)
{
}

if(feof())
{
  /* we have read all the file */
}
else
{
  /* some errors occured during parsing the file */
}

---

Quote:

---

Originally Posted by icepie

Yes it's ANSI C, so C99
I was told its not good to use break, because of structured programming rules. Would it be better to add to the while condition, even if it makes it unreadable?

---

No. 'break' is structured programming. It's very much the right thing to use in a lot of circumstances, and anyone who says otherwise is an idiot. I'd hate to think what such a person would think of exceptions!
The alternative to using break statements leads to worse programming sins such as code duplication and writing unnecessarily complex conditional expressions. All you'll do is annoy people who have to use that code, and introduce more bugs than you would simply using 'break'.