Producer Consumer in C

Question

I'm doing multithreading at university and we've been asked to implement the producer consumer problem in C.

My code compiles and produces the right result. From debugging it, it seems to be waiting for the semaphores to be freed etc. I'm using Xcode 6.4, hence the need for sem_open() instead of sem_init(). I just have a few questions regarding my code.

1. Have I done it correctly? Are there any mistakes/bugs?
2. If I wanted to make buff larger than one, would I set the semaphore fre to have an initial value of size buff? In the producer thread, would I have to call sem_wait(fre) bufftimes before I called sem_post(busy)? And then a similar thing in the consumer thread?

This is my code:

#include <stdio.h>
#include <stdlib.h>
#include <semaphore.h>
#include <pthread.h>
#include <errno.h>
#include <string.h>

void * producer(void * b);
void * consumer(void * b);

void error(char msg) {
    perror(&msg);
    exit(1);
}

sem_t * fre, * busy;

int size_buff = 1;
char message[] = "Hello!";
int size_msg = sizeof(message);

int main(int argc, const char * argv[]) {

    //declare threads
    pthread_t prod,con;
    int r;

    //create buffer and place it on the stack
    char * buff;
    buff = (char *) malloc(sizeof(char) * size_buff);


    //declare semaphores and initilise them
    char prod_name[] = "/Prod";
    char con_name[] = "/Con";
    fre = sem_open(prod_name, O_CREAT, 0777, 1);
    if ((int) fre == -1)
        error(errno);
    busy = sem_open(con_name, O_CREAT, 0777, 0);
    if ((int) fre == -1)
        error(errno);

    //start threads
    r = pthread_create(&prod, NULL, producer, (void *) buff);
    if (r) {
        error(r);
    }
    r = pthread_create(&con, NULL, consumer, (void *) buff);
    if (r) {
        error(r);
    }


    //unlink semaphores
    sem_unlink(prod_name);
    sem_unlink(con_name);

    pthread_exit(NULL);
    return 0;
}

void * producer(void * b) {
    char * buff = (char *) b;
    int count = 0;

    //loop through message[] and write elements to buffer
    while (count < size_msg) {
        sem_wait(fre);
        strncat(buff,&message[count],1);
        sem_post(busy);
        count++;
    }

    return NULL;
}


void * consumer(void * b) {
    char * recv;
    recv = malloc(sizeof(char) * size_msg);
    char * buff = (char *) b;
    int count = 0;

    //read from the buffer
    while (count < size_msg) {
        sem_wait(busy);
        strncat(recv, buff, 1);
        memmove(buff, buff + 1, 1); //move the elements in the buffer along one, essentailly deleting the element just read
        sem_post(fre);
        count++;
    }
    printf("%s",recv);
    return NULL;
}

Answer 1

When calling the function malloc():

1. The returned type is void*, so it can be assigned to any pointer, making the 'cast' totally unneeded. It just clutters the code, making it more difficult to understand, debug, maintain.
2. The expression sizeof(char) is defined in the standard at 1. Multiplying anything by 1 has no effect, so the expression just clutters the code.
3. When calling any of the memory allocation functions (malloc, calloc, realloc), always check (!=NULL) the returned value to assure the operation was successful.

When invoking the main() function and the code does not use the parameters, use the signature int main(void).

When compiling, always enable all the warnings, then fix those warnings. (For gcc, at a minimum use: -Wall -Wextra -pedantic I also use: -Wconversion -std=gnu99.)

For ease of understanding and readability by the reader of the code:

1. Separate code blocks (for, if, else, while, do...while, switch, case, default) via a blank line.
2. Use meaningful variable names. A variable name should indicate usage or content (or better, both).

The code is using 'named' semaphores. 'named' semaphores are persistent, so calling sem_open() with the O_CREAT parameter will fail if the file has ever been run before. I suggest first calling sem_unlink().

The calls to error() are not correct. The passed parameter is errno, a negative number, not a char and will not tell the user anything useful, especially if what is displayed is the contents of memory those address corresponds to the errno value. I suggest calling syserror() to get the error message that the OS thinks caused the error.

The call to pthread_create() returns 0 on success and -1 on failure. Passing that -1 to the function error() is meaningless.

I suggest passing a char* similar to error("pthread_create for producer failed") then modifying the error() function to expect a pointer to a char array.

The call to sem_open() returns a value SEM_FAILED which might be equal to -1 but that is an implementation detail, so do not use -1, use SEM_FAILED.

The second call to sem_open() returns busy, so the error check following that call should be checking busy, not free.

There are several other problems with the code, but the above comments should get you moving in the right direction.