Example of a Multithreaded C program

In answering password cracker in c with multithreading, I ended up writing a sample in C, which is not my forte.

Is there anything that I missed which should have been included in a responsible sample (read: likely to be copied and pasted into production code)?

``
// compile with gcc counter.c -o counter -lpthread -Wall -std=gnu99`
#include
#include
#include
#include

#define SEARCH_THREAD_COUNT (10)
#define SEARCH_VALUE (21325)
#define SEARCH_MAX (INT_MAX)

typedef struct {
int start;
int end;
int search;

bool* shouldStop;
int* answer;
pthread_mutex_t* answerMutex;
pthread_cond_t* answerFound;

pthread_t thisThread;
} find_worker_init;

void find_value(void vpInitInfo) {
find_worker_init pUnitOfWork = (find_worker_init)vpInitInfo;

int cValue = pUnitOfWork->start;
while (!(*(pUnitOfWork->shouldStop))) {
if (cValue == pUnitOfWork->search) {
printf("found value\n");

// found the search value
pthread_mutex_lock(pUnitOfWork->answerMutex);
if (!(*(pUnitOfWork->shouldStop))) {
*(pUnitOfWork->shouldStop) = true;
*(pUnitOfWork->answer) = cValue;

pthread_cond_broadcast(pUnitOfWork->answerFound);
}
pthread_mutex_unlock(pUnitOfWork->answerMutex);
return NULL;
}

cValue++;
if (cValue == pUnitOfWork->end) {
// we exhausted our search space, end.
printf("exhausted\n");
return NULL;
}
}

// we were usurped by another thread
printf("usurped\n");
return NULL;
}

int main( int argc, const char* argv[] ) {
pthread_mutex_t answerMutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t answerFound = PTHREAD_COND_INITIALIZER;
bool shouldStop = false;
int answer = -1;

// initialize thread jobs
find_worker_init startInfo[SEARCH_THREAD_COUNT];
int current_search_start = 0;
int search_un

Mitch, you have a nice program there. As I pointed out above, current_search_start is not updated which means 9 of 10 threads are searching the same space and the last searches the whole space. Minor error though.

Your use of condition variables and mutexes is nicely done. Overall, nothing significant to comment on.

Some minor comments:

-
The precedence of -> is higher than that of * so this:

if (!(*(pUnitOfWork->shouldStop))) {

can be written:

if (!(*pUnitOfWork->shouldStop)) {

or some might prefer:

if (*pUnitOfWork->shouldStop == false) {

And

*(pUnitOfWork->shouldStop) = true;

can omit the parenthesis and be neater.

-
I would change some variable names. cValue seems meaningless. pUnitOfWork would be just work. The p (and vp) prefixes you used look like Hungarian. I would never use such things unless forced to by an outdated coding convention.

-
find_value should strictly be static

-
The loop in find_value would be more normal as a for:

for (int c = work->start; c end; ++c) {
    if (*work->shouldStop) { ... }
    if (c == work->search) { ... }
}

Also I would add a thread number and some debug to see which thread number is successful (especially with the current_search_start bug, as the thread is not always the same :-)

StackExchange Code Review Q#64839, answer score: 3