Barry Schwarz
6/27/2011 1:05:00 PM
On Sun, 26 Jun 2011 20:50:21 -0700 (PDT), Vinicio Flores
<vfloreshdz@gmail.com> wrote:
>Hello C hackers, this program that I am writing print "Segmentation
>fault" when I execute it, I think that the problem is the gets
>function, but I am not sure
Your functions do not play nicely together. You have too many calls
to fopen() and the modes do not match the intended use.
>
>/** A simple database system **/
>
>#include "datbase.h"
>
>static int search_and_open_data_base(char *Arg)
This function does not do any searching.
>{
> FILE *f;
Don't you need to #include stdio.h for FILE and its associated
functions such as fopen?
>
> f = fopen(Arg, "r");
>
> if(!f)
> return 1;
> else
> return 0;
Now you have an open file and no FILE* that points to it. You can
never close it. (This is a possible memory leak depending on how your
system allocates files.) On some system, a second attempt to open the
file for output will fail because it is already open for input.
>}
>
>/* ************************************************ */
>
>static FILE *create_database(char *arg)
>{
> FILE *newbase;
>
> newbase = fopen(arg, "w+");
>
> return newbase;
>}
>
>/* ************************************************ */
>
>static int print_options()
>{
> clrscr();
> setattr(REVERSE);
> setcolor(FOREBLUE);
>
> gotoxy(2, 2);
> cprintf("DatBASE");
That is a strange use of camel case.
You use a lot of non-standard functions (possibly POSIX) but you don't
seem to have the correct headers for them. In any event, this limits
the number of people who can attempt to help to just those who have
the same or a similar system. If that is essential to understanding
the problem, you might be better off posting in a group that deals
with that system.
In this case, why use a non-standard function which does nothing
different than a standard function?
>
> gotoxy(2, 4);
> setattr(NORMAL);
> setcolor(FOREGREEN);
> cprintf("I)Insert new register R)Print a register S) Exit");
Most menus seem to use P for Print and X for eXit.
>
> gotoxy(2, 6);
> setcolor(FORERED);
> cprintf("Option: ");
>
> setattr(NORMAL);
> normvideo();
>
>}
>
>/* ************************************************ */
>
>static int open_and_fill_database(char *arg)
>{
> FILE *database;
>
> struct datbase_registers *base = malloc ( sizeof(struct registers
>*));
Others have pointed out the error here.
>
> database = fopen(arg, "ra");
ra is not a standard mode. Read and append makes no sense.
>
> gotoy(10);
> puts("Insert register name: ");
> gets(base->regname);
Perhaps the only time no one will ever mention gets() overrunning the
target array. When you fix the size of your arrays, fgets() will be a
better choice.
>
> gotoy(11);
> puts("Insert name: ");
> gets(base->name);
>
> gotoy(12);
> puts("Insert telephone number: ");
> scanf("%ld", base->ntel);
Storing a telephone number as an integer can produce misleading
results if phone numbers start with 0 (as in Europe and Japan).
>
> gotoy(13);
> puts("Insert age: ");
> scanf("%ld", &base->age);
You really expect age to exceed 65K?
>
> fprintf(database, "Register name: ");
> fputs(base->regname, database);
If you are going to bear the expense of calling fprintf, why not
include everything in one function call
fprintf(database, "Register name: %s\n", base->regname);
and eliminate the call to fputs?
>
> fprintf(database, "\n\nPerson's name: ");
> fputs(base->name, database);
>
> fprintf(database, "\nTelephone number: %lf\n", base->ntel);
Here you invoke undefined behavior. ntel is an integer, not a
floating point. Did you mean %ld?
>
> fclose(database);
You never stored age in database.
>}
>
>/* ************************************************ */
>
>static int get_and_process_input()
>{
> int ch;
>
> ch = getch();
The last input could have been age. scanf() would have left the '\n'
from the ENTER key sitting in the input buffer. That is the character
that this getch() would pick up. Not what you want.
>
> switch(ch)
> {
> case 'i':
> case 'I':
> open_and_fill_database("data.base");
> break;
> case 'r':
> case 'R':
> break;
> case 's':
> case 'S':
> setattr(NORMAL);
> normvideo();
> reset();
> ret;
What kind of statement is this? Is it supposed to be a function call:
ret();
> break;
> default:
> gotoy(9);
> cprintf("?Unknown operation\n");
> sleep(1);
> print_options();
> get_and_process_input();
Did you really mean to be recursive? Wouldn't a while loop be much
easier to debug?
> break;
> }
More undefined behavior. This function is defined to return an int.
You don't. Didn't your compiler generate a diagnostic for this? If
not, maybe you should up the warning level.
>}
>
>/* ********************|*************************** */
>/* --------------------|--------------------------- */
>/* ************| MAIN FUNCTION |******************* */
>
>int main()
>{
> FILE *database;
You really need to use this variable somewhere.
>
> int c;
> int search = search_and_open_data_base("data.base");
>
>
> if(search == 1)
> create_database("data.base");
You created the file but threw away the FILE* the function returned.
You now have an empty file opened for output that you have no way to
process or close.
>
> else
> {
At this point, the file is open for input.
> print_options();
> get_and_process_input();
This may call open_and_fill_database() which will always attempt to
open data.base even if you change the file name later. This is one
occasion where having pointer to (or array containing) the file name
at file scope would be not be unreasonable, especially if it were
const.
> }
> return 0;
>}
>
>
>Here is the datbase.h C header:
>/** A simple database system **/
>
>#include <ttk.h>
>#include <unistd.h>
>#include <sys/types.h>
>#include <sys/stat.h>
>#include <fcntl.h>
>
>#define MAX ((900*800)*256)
What benefit do you perceive from the extraneous parentheses?
>
>struct datbase_registers
>{
> char regname[MAX];
> char name[MAX];
> long int ntel;
> long int age;
>};
--
Remove del for email