I'm having a hard time having a struct work using multiple files. Please help?

Question

I am trying to solve a consumer/producer problem, and I have to create three different classes.

The main class includes the creation of threads, and consumer/producer logic

the other two classes are

A header file for a ring buffer
A file containing the implementation of the ring buffer

I'm getting the following errors when trying to compile:

ringbuf.c: In function ‘rb_init’:
ringbuf.c:10: warning: incompatible implicit declaration of built-in function ‘malloc’
ringbuf.c:10: error: invalid application of ‘sizeof’ to incomplete type ‘struct ringbuf_t’ 
ringbuf.c:12: error: dereferencing pointer to incomplete type

I many other errors, but I can handle them myself once I get through this one.

this is the header file for the buffer:

struct ringbuf_t 
{
    pthread_mutex_t mutex; /* lock to protect from simutaneous access to the buffer */
    pthread_cond_t cond_full; /* producer needs to wait when the buffer is full */
    pthread_cond_t cond_empty; /* consumer needs to wait when the buffer is empty */
    int bufsiz; /* size of the buffer; you may use an empty slot to differentiate the situation the buffer is full or empty */
    int front; /* index of the first element */
    int back; /* index next to the last element (or index to the first empty slot) */
    int count; //keeps track of the number of elements in the buffer
    char* buf; /* buffer itself */
};

/* return the pointer to the newl created and initialized ring buffer of the given size */
extern struct ringbuf_t* rb_init(int bufsiz);

/* reclaim the ring buffer */
extern void rb_finalize(struct ringbuf_t* rb);

/* return the current number of elements in the buffer */
extern int rb_size(struct ringbuf_t* rb);

/* return non-zero (true) if the buffer is currently full */
extern int rb_is_full(struct ringbuf_t* rb);

/* return non-zero (true) if the buffer is currently empty */
extern int rb_is_empty(struct ringbuf_t* rb);

/* insert (i.e., append) a character into the buffer; if the buffer is full, the caller thread will be blocked */
extern void rb_insert(struct ringbuf_t* rb, int c);

/* retrieve a character at the front of the ring buffer; if the buffer is empty, the caller thread will be blocked */
extern int rb_remove(struct ringbuf_t* rb);

and this is the implementation of the buffer:

#include <malloc.h>
#include <stdio.h>

struct ringbuf_t 
{
    pthread_mutex_t mutex; /* lock to protect from simutaneous access to the buffer */
    pthread_cond_t cond_full; /* producer needs to wait when the buffer is full */
    pthread_cond_t cond_empty; /* consumer needs to wait when the buffer is empty */
    int bufsiz; /* size of the buffer; you may use an empty slot to differentiate the situation the buffer is full or empty */
    int front; /* index of the first element */
    int back; /* index next to the last element (or index to the first empty slot) */
    int count; //keeps track of the number of elements in the buffer
    char* buf; /* buffer itself */
};


struct ringbuf_t* rb_init(int bufsiz)
{
    struct ringbuf_t* buffer = (struct ringbuf_t*)malloc(sizeof(struct ringbuf_t));

    buffer->bufsiz = bufsiz;
    buffer->front = 0;
    buffer->back = 0;
    buffer->count = 0;
}


/* reclaim the ring buffer */
void rb_finalize(struct ringbuf_t* rb)
{
    free(rb);
    pthread_mutex_destroy(&rb->mutex);
    printf("\nnotice - ring buffer finalized");
}



/* return the current number of elements in the buffer */
int rb_size(struct ringbuf_t* rb)
{
    return (rb->count);
}



/* return non-zero (true) if the buffer is currently full */
int rb_is_full(struct ringbuf_t* rb)
{
    return ((rb->count) == rb->bufsiz);
}



/* return non-zero (true) if the buffer is currently empty */
int rb_is_empty(struct ringbuf_t* rb)
{
    return ((rb->count) == 0);
}



/* insert (i.e., append) a character into the buffer; if the buffer is full, the caller thread will be blocked */
void rb_insert(struct ringbuf_t* rb, int c)
{
    char* temp;


    if(rb->count < rb->bufsiz)
    {
        if(rb->count == 0)
        {
            rb->front = 0;
            rb->back = 0;
            rb->buf = c;
            rb->count++;
        }
        else
        {
            if(rb->front < (rb->bufsiz - 1))
            {
                temp = rb->buf;
                temp = temp + rb->front + 1;
                temp = c;
                rb->front++;
            }
            else if(rb->front == (rb->bufsiz -1))
            {
                rb->front = 0;
                rb->buf = c;
            }
        }
    }
    else
    {
        printf("\nerror - trying to insert into full buffer");
    }
}



/* retrieve a character at the tail (back) of the ring buffer; if the buffer is empty, the caller thread will be blocked */
int rb_remove(struct ringbuf_t* rb)
{
    if(rb->count != 0)
    {
        count--;
        if(rb->back < (rb->bufsiz-1)
        {
            rb->back++;
        }
        else if(rb->back == (rb->bufsiz -1))
        {
            rb->back = 0;
        }
    }
    else
    {
        printf("\nerror - trying to remove from empty buffer");
    }

}

this is the main class:

#include <stdio.h>
#include <pthread.h>
#include <ringbuf.h>


    //creating a static ring buffer
struct ringbuf* mybuffer = rb_init(10);

int thisChar;

    /*
     consumer thread, reads one character at a time, sets the lock when addinga character to the ring buffer
     while locked it checks if the buffer is full, waits for a slot, and then continues.
     */
void* consumer(void* arg)
{
    printf("consumer started");

    while(thisChar != EOF)
    {
        pthread_mutex_lock(&(mybuffer->mutex));

        while(rb_is_empty(mybuffer))
            pthread_cond_wait(&(mybuffer->cond_full), &(mybuffer->mutex));

        printf("%s", (char)rb_remove(mybuffer));

        pthread_cond_signal(&(mybuffer->cond_empty));

        pthread_mutex_unlock(&(mybuffer->mutex));
    }
}

    /*
     producer thread, takes one character at a time from the buffer, (while the buffer is not empty)
     and prints it to the screen.
     */


void* producer(void* arg)
{
    printf("producer started");

    while ((thisChar = getchar()) != EOF)
    {

        pthread_mutex_lock(&(mybuffer->mutex));

        while(rb_is_full(mybuffer))
            pthread_cond_wait(&(mybuffer->cond_empty), &(mybuffer->mutex));

        rb_insert(mybuffer, thisChar);

        pthread_cond_signal(&(mybuffer->cond_full));

        pthread_mutex_unlock(&(mybuffer->mutex));
    }
}


int main()
{

        //declaring threads
    pthread_t t0, t1;


        //creating threads as condumer, producer
    p_thread_create(&t0, NULL, consumer, (void*)mybuffer);
    p_thread_create(&t1, NULL, producer, (void*)mybuffer);


    pthread_join(t0, NULL);
    pthread_join(t1, NULL);

    rb_finalize(mybuffer);

    return 0;
}

I'm missing some stuff, but I need to get through this first! please help!

The implementation file should #include the header file. – David Schwartz Nov 07 '11 at 05:29 — David Schwartz, Nov 07 '11 at 05:29

score 6 · Answer 1 · answered Nov 07 '11 at 05:23

6

Replace your #include <malloc.h> lines with #include <stdlib.h>. That will fix the errors you have pasted here (and probably many many more). When you do that, go back through your code and remove all the casts in your calls to malloc(3):

struct ringbuf_t* buffer = (struct ringbuf_t*)malloc(sizeof(struct ringbuf_t));

That (struct ringbuf_t*) hasn't been necessary since roughly 1989, when function prototypes were pushed into the language.

answered Nov 07 '11 at 05:23

sarnold

102,305
22
181
238

It (the cast) is still necessary if the code will be compiled by a C++ compiler. Granted, the question is about C, but funnier things have been known. – Jonathan Leffler Nov 07 '11 at 07:39
@Jonathan, obviously the OP is learning C, as you also clearly seem to assume in your detailed answer. Casting the return from `malloc` is one of the things that bites here. – Jens Gustedt Nov 07 '11 at 08:32
@Jonathan, thanks, I've found again and again that my C knowledge doesn't exactly carry over to C++ -- I was completely unaware that the cast is still required in C++. – sarnold Nov 07 '11 at 08:48
@Jens, it was news to me that one must cast the return from `malloc()` in C++. :) – sarnold Nov 07 '11 at 08:49
2

@sarnold, I think your compiler would have told you :) You just shouldn't use `malloc` in C++ in the first place. In essence this tells you that C and C++ are similar languages that can be made declaration compatible (for header files) but have incompatibilities that make it almost impossible to use clean code that is written for one of them to be used by the other. – Jens Gustedt Nov 07 '11 at 09:14

score 3 · Accepted Answer · edited May 23 '17 at 11:47

ringbuf.h

Your ringbuf.h header should be self-contained and idempotent. It should, therefore, include <pthread.h>.

#ifndef RINGBUF_H_INCLUDED
#define RINGBUF_H_INCLUDED

#include <pthread.h>

struct ringbuf_t 
{
    pthread_mutex_t mutex; /* lock to protect from simutaneous access to the buffer */
    pthread_cond_t cond_full; /* producer needs to wait when the buffer is full */
    pthread_cond_t cond_empty; /* consumer needs to wait when the buffer is empty */
    int bufsiz; /* size of the buffer; you may use an empty slot to differentiate the situation the buffer is full or empty */
    int front; /* index of the first element */
    int back; /* index next to the last element (or index to the first empty slot) */
    int count; //keeps track of the number of elements in the buffer
    char* buf; /* buffer itself */
};

/* return the pointer to the newl created and initialized ring buffer of the given size */
extern struct ringbuf_t* rb_init(int bufsiz);

/* reclaim the ring buffer */
extern void rb_finalize(struct ringbuf_t* rb);

/* return the current number of elements in the buffer */
extern int rb_size(struct ringbuf_t* rb);

/* return non-zero (true) if the buffer is currently full */
extern int rb_is_full(struct ringbuf_t* rb);

/* return non-zero (true) if the buffer is currently empty */
extern int rb_is_empty(struct ringbuf_t* rb);

/* insert (i.e., append) a character into the buffer; if the buffer is full, the caller thread will be blocked */
extern void rb_insert(struct ringbuf_t* rb, int c);

/* retrieve a character at the front of the ring buffer; if the buffer is empty, the caller thread will be blocked */
extern int rb_remove(struct ringbuf_t* rb);

#endif /* RINGBUF_H_INCLUDED */

Were it my header, I'd have an extra line:

typedef struct ringbuf_t ringbuf_t;

and I'd edit the function prototypes to lose the struct keyword.

The advantage of this is that anyone can include ringbuf.h and it will simply work for them.

ringbuf.c

It is crucial that the implementation file uses its own header; that gives you the necessary cross-checking that the header accurately reflects what is implemented. It should also be the first header included; this gives a simple but effective check that the header is self-contained.

You should not use <malloc.h> unless you are using its extended features. The <stdlib.h> declares malloc() et al and should be used unless you know which extra functions are available in <malloc.h> and you actually use them.

This leads to:

#include "ringbuf.h"
#include <stdio.h>
#include <stdlib.h>

struct ringbuf_t* rb_init(int bufsiz)
{
    struct ringbuf_t* buffer = (struct ringbuf_t*)malloc(sizeof(struct ringbuf_t));

    buffer->bufsiz = bufsiz;
    buffer->front = 0;
    buffer->back = 0;
    buffer->count = 0;
}

/* reclaim the ring buffer */
void rb_finalize(struct ringbuf_t* rb)
{
    free(rb);
    pthread_mutex_destroy(&rb->mutex);
    printf("\nnotice - ring buffer finalized");
}

/* return the current number of elements in the buffer */
int rb_size(struct ringbuf_t* rb)
{
    return (rb->count);
}

/* return non-zero (true) if the buffer is currently full */
int rb_is_full(struct ringbuf_t* rb)
{
    return ((rb->count) == rb->bufsiz);
}

/* return non-zero (true) if the buffer is currently empty */
int rb_is_empty(struct ringbuf_t* rb)
{
    return ((rb->count) == 0);
}

/* insert (i.e., append) a character into the buffer; if the buffer is full, the caller thread will be blocked */
void rb_insert(struct ringbuf_t* rb, int c)
{
    char* temp;

    if(rb->count < rb->bufsiz)
    {
        if(rb->count == 0)
        {
            rb->front = 0;
            rb->back = 0;
            rb->buf = c;
            rb->count++;
        }
        else
        {
            if(rb->front < (rb->bufsiz - 1))
            {
                temp = rb->buf;
                temp = temp + rb->front + 1;
                temp = c;
                rb->front++;
            }
            else if(rb->front == (rb->bufsiz -1))
            {
                rb->front = 0;
                rb->buf = c;
            }
        }
    }
    else
    {
        printf("\nerror - trying to insert into full buffer");
    }
}

/* retrieve a character at the tail (back) of the ring buffer; if the buffer is empty, the caller thread will be blocked */
int rb_remove(struct ringbuf_t* rb)
{
    if(rb->count != 0)
    {
        count--;
        if(rb->back < (rb->bufsiz-1)
        {
            rb->back++;
        }
        else if(rb->back == (rb->bufsiz -1))
        {
            rb->back = 0;
        }
    }
    else
    {
        printf("\nerror - trying to remove from empty buffer");
    }
}

You should probably use fprintf(stderr, ...) rather than printf() for diagnostics, and you should consider how to turn them off at run-time (or, more likely, how to turn them on).

Note that it is conventional to put system-provided headers in angle brackets (hence <stdio.h>) and user-provided headers in double quotes (hence "ringbuf.h").

Your rb_init() function should initialize the structure completely. That means that the mutex and the two condition variables should both be initialized properly. It also needs to either initialize (zero) the buf member or allocate the appropriate amount of space - more likely the latter. Your code should check that the allocations succeed, and only use the allocated space if it does.

You should also review whether it is appropriate to make the producer and consumer threads manipulate the mutex and condition variables. If they are bundled with the structure, the functions bundled with the structure should do what is necessary with the mutexes and conditions. This would allow you to simplify the producer and consumer to just call the ring buffer functions. Clearly, the main() will still launch the two threads, but if you get your abstraction right, the threads themselves won't need to dink with mutexes and conditions directly; the ring buffer library code will do that correctly for the threads. One of the advantages of this is that your library can get the operations right, once, and all consumers benefit. The alternative is to have every producer and consumer handle the mutexes and conditions - which magnifies the opportunities to get it wrong. In a classroom situation where you won't use the abstraction again after this exercise, the proper separation and encapsulation is not so critical, but in professional code, it is crucial that the library make it easy for people to use the code correctly and hard for them to make mistakes.

main.c

In C, you cannot initialize a global variable with a function call - in C++, you can.

Hence, this won't compile in C:

//creating a static ring buffer
struct ringbuf_t *mybuffer = rb_init(10);

You should use:

struct ringbuf_t *mybuffer = 0;

and then, in main() or a function called from main() - directly or indirectly - do the function call:

mybuffer = rb_init(10);

This would be before you do any work creating the threads. When your rb_init() code initializes the mutex and condition variables, your main() will be able to go ahead as written.

Until then, you have a good deal of cleanup to do.

Disclaimer I have not compiled the code to see what the compiler witters about.

Note If you use GCC but don't compile with at least -Wall and preferably -Wextra too (and clean up any (all) the warnings), you are missing out on a very important assistant. I work with retrograde code bases where I have to worry about -Wmissing-prototypes -Wold-style-definition -Wstrict-prototypes too. Using -Werror can be helpful; it forces you to clean up the warnings.

Thanks a lot! I fixed most of my problems with your help! – Paradox Nov 08 '11 at 01:17 — Paradox, Nov 08 '11 at 01:17

I'm having a hard time having a struct work using multiple files. Please help?

2 Answers2

ringbuf.h

ringbuf.c

main.c