passing pointer to recursive function in C

Question

I'm just starting on the road the learning C, and ran into some difficulty:

The code listed below is giving me the following error:

Attaching to program: `/workfolder/cocoa/c_stuff/bookshelf/build/Debug/bookshelf', process 1674.
Cannot access memory at address 0xa0df194
Cannot access memory at address 0xa0df194

// code start

#define MAX_NAME_LENGTH 200
#define MAX_AUTHOR_LENGTH 200
#define MAX_DESCRIPTION_LENGTH 1000
#define MAX_PUBLISHER 200
#define MAX_ISBN 50


//structures<
typedef struct {
    char title[MAX_NAME_LENGTH];
    char author[MAX_AUTHOR_LENGTH];
    char ISBN[MAX_ISBN];
    char description[MAX_DESCRIPTION_LENGTH];
    char publisher[MAX_PUBLISHER];
} Book;


void getUserInput(Book *s[])
{   
    printf("what is the book's title ?\n");
    fgets(s[book_count]->title, MAX_NAME_LENGTH, stdin);

    printf("what is the author's name?\n");
    fgets(s[book_count]->author, MAX_AUTHOR_LENGTH, stdin);

    printf("what is the ISBN?\n");
    fgets(s[book_count]->ISBN, MAX_ISBN, stdin);

    printf("write a short description\n");
    fgets(s[book_count]->description, MAX_DESCRIPTION_LENGTH, stdin);

    printf("what is the book's publisher\n");
    fgets(s[book_count]->publisher, MAX_PUBLISHER, stdin);

    printf("want to add another book ? Y\\N\n");

    book_count++;

    if(tolower(fgetc(stdin)) == 'y') 
    {
     return getUserInput(s);
    } 
    else 
    {
     return;
    }
}


int main (int argc, const char * argv[]) {
    // insert code here...
    Book *book_shelf[100];

    if((book_shelf[0] = (Book *)malloc(sizeof(Book))) == NULL)
    {
     exit(1);
    }

    getUserInput(book_shelf);

    return 0;
}

The code compiles properly, and the function runs fine the first time (all the questions get asked and the struct receives the data); but when the user types 'y' to add another book, the mem error occurs.

Any ideas where the error is happening?

Thanks in advance!

Answer 1

You've only ever allocated memory for the first book in main - after that it tries to write to the next slot in the array, which doesn't point to an allocated block of memory, giving you a seg-fault. You're going to have to allocate memory for each book you want to read in.

In addition, since C doesn't know how long an array is, you have to pass that information along into function calls. (And I don't see where you're defining book_count.)

You might try something along these lines:

void getUserInput(Book *s[], int *book_count, int max_book_count)
{
   if (book_count == max_book_count) return; // If we've filled all the slots, we can't add anymore without causing trouble.
   s[book_count] = malloc(sizeof(Book));

   ..

   if(tolower(fgetc(stdin)) == 'y') 
   {
       (*book_count)++;
       getUserInput(s, book_count, max_book_count);
   } 
   return;
}

int main (int argc, const char * argv[]) {
    // insert code here...
    Book *book_shelf[100];

    int book_count = 0;
    getUserInput(book_shelf, &book_count, 100);
    // Make sure to free all the malloc'd data
}

Even better in this situation, would just be using a loop and skipping the whole recursion step.

int main (int argc, const char * argv[]) {
    // insert code here...
    Book *book_shelf[100];

    char response = 'y';
    int book_count = 0;
    while (book_count < 100 && response == 'y')
    {
        book_shelf = malloc(sizeof(Book));
        response = getUserInput(book_shelf[book_count++]);
    }
    // make sure to free all the allocated data!
}

char getUserInput(Book *book)
{
   // write input straight to book
   printf("what is the book's title ?\n");
   fgets(book->title, MAX_NAME_LENGTH, stdin);

   ...

   return tolower(fgetc(stdin));
}

Answer 2

Unless I'm reading something wrong, you haven't defined book_count before using it as an array subscript.

Answer 3

Within main, you allocated on the stack an array of 100 pointers to the Book Structure. I believe it was your intent to allocate 100 structures and then pass the address to that block of structures to getUserInput

Change main to:

Book book_shelf[100];
...
getUserInput(book_shelf);
...

EDIT: OOPS Missed the single Book malloc mentioned in the earlier post. That ones Correct for the first book. If you edit as above and eliminate the if (book_shelf[0]...) check, you'll accomplish your intended results

Answer 4

1. You allocate just space for the firstbook, not for the others (malloc in main)

2. I guess there is some code missing, no declaration and initialization of book_count

3. You should use loops instead of recursion

4. Use not recursion but loops for this kind of repetition

Answer 5

As in Josh's answer, by adding the following lines to your code should make it work:

book_count++;

if(tolower(fgetc(stdin)) == 'y') 
{
    if((book_shelf[book_count] = (Book *)malloc(sizeof(Book))) == NULL)
    {
        printf("Cannot allocate memory for Book");
        exit(1);
    }
    return getUserInput(s);
} 
else 
{
    return;
}

However, I encourage you not to use the recursive function for getting input. Recursive can lead to difficulties in debugging. You may consider using normal loop instead.

Note: I'm assuming the book_count is global variable which has been initialized to 0

Answer 6

Recursion is probably overkill for this problem where a simple do { ... } while(user keeps answering yes) would do. However the problem you having is in main with your Book *book_shelf[100]. There are several ways you could solve this problem.

First change it to an array of Book's like samills suggests:

Book book_shelf[100];

and then change your getUserInput to something like this:

getUserInput(Book *book_shelf, int offset, int length) {
    if(offset < 0 || offset >= length) {
        return;
    }

    //...

    return getUserInput(book_shelf, offset + 1, length)
}

Or you could use your existing code and change you getUserInput function to look something like this and remove the malloc from main:

getUserInput(Book *book_shelf) {
     book_shelf[book_count] = (Book*)malloc(sizeof(Book));
     // ...
}

props for correct use of the sizeof operator (I see that thing misused so often it makes my eyes bleed).

Answer 7

thanks a lot for the replies!

I realized that I hadn't malloc-ed enough memory to handle more then one element of the struct array (Exactly what Josh is saying). So essentially:

Book *book_shelf;

if(book_shelf = (Book*)malloc(sizeof(Book)) == NULL)//exit code

so the second time around I would hit a memory issue.

thanks again!

Answer 8

Looks like your still doing it wrong:

Book *book_shelf;

if(book_shelf = (Book*)malloc(sizeof(Book)) == NULL)//exit code

book_shelf is only the size of a pointer. When you do the malloc you only allocate one Book at a time. This is wrong. You need to allocate contiguous memory for an array of Book objects all in one instanciation of an array.

Like

Book book_shelf[100];

not

Book *book_shelf[100];

or using malloc, use your pointer to point to an array instanciated using
100*malloc(sizeof(Book)).

You may get lucky that no other heap memory is allocated in between your malloc(sizeof(Book)) calls and that the memory management system is alocating contiguous memory by default. Also, book_shelf will only point to the last malloced Book structure, not the first one as you indicated you want in your original question.

Josh is also not allocating enough memory at one time. Use a linked list if you want to keep extending elements to the end of your book_shelf one-by-one.