3
\$\begingroup\$

Can you review the code?

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

#include "bin/include/pthread.h"
#include "bin/include/sched.h"
#include "bin/include/semaphore.h"


#define TCOUNT 10
#define WATCH_COUNT 12

int count = 0;
pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t count_threshold_cv = PTHREAD_COND_INITIALIZER;
int  thread_ids[2] = {1,2};

/* Function prototypes */
void watch_count(int *idp);
void inc_count(int *idp);
int getDirFileCount(void);
/* End of fuction prototypes */


int main(void)
{
    pthread_t threads[2];
    int retCode = 0;
    int initialFileCount = getDirFileCount();
    count = initialFileCount;
    printf("No of Files initially : %d \n",initialFileCount);
    if ((retCode = pthread_create(&threads[0],NULL,(void *)&inc_count, &thread_ids[0]))) {
        perror("Thread creation error : can't able to create thread");
    }
    if ((retCode = pthread_create(&threads[1],NULL,(void *)&watch_count, &thread_ids[1]))) {
        perror("Thread creation error : can't able to create thread");
    }

    pthread_exit(NULL);
    return 0;
}

void watch_count(int *idp)
{
    pthread_mutex_lock(&count_mutex);
    //int watchCount = getDirFileCount();
    while (1) {
        printf("waiting for changes made to the directory \n");
        pthread_cond_wait(&count_threshold_cv,
                          &count_mutex);
        printf("OK , now someone made chaneges to the directory\n");
    }
    pthread_mutex_unlock(&count_mutex);
}
void inc_count(int *idp)
{
    for (;;) {
        pthread_mutex_lock(&count_mutex);
        int countnow = getDirFileCount();
        printf("Number of files now: %d \n",countnow );
        if (count < countnow) {
            pthread_cond_signal(&count_threshold_cv);
            pthread_mutex_unlock(&count_mutex);
            return;
        }
        //pthread_cond_signal(&count_threshold_cv);
        pthread_mutex_unlock(&count_mutex);
        //pthread_exit(NULL)

    }
}


int getDirFileCount(void)
{
    int count = 0;
    WIN32_FIND_DATA fd;
    char *dirFilePath = "D:\\Test\\*";
    HANDLE h = FindFirstFile(dirFilePath, &fd);
    if (h != INVALID_HANDLE_VALUE) {
        do {
            count++;
        } while (FindNextFile(h, &fd));
        FindClose(h);
    }
    return count;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You tagged this question as linux, yet I see WIN32_FIND_DATA fd; char *dirFilePath = "D:\\Test\\*";which suggests that this code is for Windows? \$\endgroup\$ – 200_success Jan 23 '16 at 7:36
  • \$\begingroup\$ it is ported code , basically written in linux \$\endgroup\$ – cslrnr Jan 23 '16 at 18:05
2
\$\begingroup\$

Could be simplified

For what you are doing, using a mutex and a condition variable is more complex than necessary. You could just use a semaphore or signal instead. A mutex should guard some shared variable or state, but your code doesn't have any. Instead, you are using the mutex + condition variable simply to wake up another thread.

As far as your implementation goes, I have the following comments:

  • A loop containing pthread_cond_wait() should always check a predicate variable to guard against spurious wakeups. This is in the documentation for pthread_cond_wait(). So for example:

    pthread_mutex_lock(&count_mutex);
    while (count == initialCount) {
        pthread_cond_wait(&count_threshold_cv, &count_mutex);
    }
    // Do something here...
    pthread_mutex_unlock(&count_mutex);
    
  • The loop doing the work should lock the mutex for as short a time as possible. You only need to hold the mutex when you want to access shared variables. So for example:

    for (;;) {
        // Do this long work outside of the mutex
        int countnow = getDirFileCount();
    
        if (countnow > initialCount) {
            // Only lock the mutex when you need to actually need to access
            // shared state (in this case the variable "count").
            pthread_mutex_lock(&count_mutex);
            count = countnow;
            pthread_cond_signal(&count_threshold_cv);
            pthread_mutex_unlock(&count_mutex);
            return;
        }
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.