Dining Philosophers variation in C

This is a variation of the Dining Philosophers Problem. The task is to coordinate several students inside a gym. All students try to obtain their desired training weights from a shared weight rack. During runtime, the user can issue commands to:

• block a student (b + student id)

• unblock a student (u + student id)

• proceed (p + student id) (ends a student's workout/rest loop)

• end the program (q or Q)

I'm a beginner in C and would appreciate opinions and pointers on how to improve my code.

main.c

```
#include
#include "main.h"
#include "gym_monitor.h"
/*
*
* Main module of the Thread-Coordination Exercise. This modul contains
* the main function which creates the threads. After creation all threads
* enter the gym_routine function.
*
*/
#define REST_LOOP 1000000000
#define WORKOUT_LOOP 500000000

#define WEIGHTS_ANNA 6
#define WEIGHTS_BERND 8
#define WEIGHTS_CLARA_DIRK 12
#define WEIGHTS_EMMA 14

#define MAX_INPUT_SIZE 3

#define WEIGHT_RACK_DEF {4,4,5}

static pthread_barrier_t gym_routine_barrier;

static void workout(Student* student) {
for( int i = 0; i status == BLOCKED) {
rest_student(student);
}else if(student->status == PROCEED) {
student->status = NORMAL;
break;
}
}
}

static void rest(Student* student) {
for( int i = 0; i status == BLOCKED) {
rest_student(student);
}else if(student->status == PROCEED) {
student->status = NORMAL;
break;
}
}
}

static void gym_routine(void stud) {
pthread_barrier_wait(&gym_routine_barrier);
Student student = (Student) stud;
while(student->status != QUIT) {
get_weights(student);
workout(student);
put_weights(student);
rest(student);
}
return NULL;
}

int main(void) {

char available_weights[] = WEIGHT_RACK_DEF;

int students_weights[] = {WEIGHTS_ANNA,WEIGHTS_BERND,WEIGHTS_CLARA_DIRK,
WEIGHTS_CLARA_DIRK,WEIGHTS_EMMA};

Student students[NR_STUDENTS];
monitor_vars

Before I go into details, I feel obliged to say that your project is fairly ambitious for a C beginner. I have several criticisms, which I hope you will receive as constructive, but overall, your code makes me suspect that although you are new to C, you are not new to programming in general or to multi-threaded programming in particular. If I am mistaken about that then please take it as a complement.

Data races

Your code contains some data races involving the manipulation and testing of students' status variables. The main thread modifies these as a result of command input and at shutdown, and each student's thread both reads and writes that variable for its own student. Some of the student threads' accesses are performed under protection of the mutex (those in functions from gym_routine.c), but others and the main thread's are not.

Since these variables are written to by at least one thread each and read by multiple threads, every access must be appropriately protected once the per-student threads are started. You've apparently chosen to use a mutex for that, which is fine; you just need to be sure to protect all accesses.

Busy loops

You use high-iteration-count busy loops for making the workout() and rest() functions consume non-trivial time. At minimum, you'll need to greatly reduce the iteration counts when you correct the data races there, as locking and unlocking mutexes is costly.

Really, however, you ought to choose a delay mechanism that doesn't consume CPU. pthread_cond_timedwait() provides one such mechanism, with the advantage that another thread (e.g. the main one) can interrupt the wait if needed. That could be made to work in concert with resolving your data races, by giving each student its own mutex and condition variable to protect access to the student status. That would also allow you to set the durations of the workout and rest times in terms of machine-independent time units.

Unnecessary dynamic allocation

There are many good uses for dynamic allocation, but it's complicated enough and easy enough to mismanage that you should not use it where you don't actually need it. In particular, just because you need a pointer to something does not necessarily mean that that thing needs to be dynamically allocated. It's not uncommon to use a pointer to an ordinary local or file-scope variable, obtained via the & operator.

In your code, this applies to most (but, oddly, not all) of your synchronization objects. For example, I recommend changing the sem_student member of struct Student from a pointer to a plain sem_t. (It will then need to be handled slightly differently, but mainly the dynamic allocation will go away.) Similarly, there is no need to dynamically allocate your monitor_vars object. Just declare an instance.

Input handling

In your main input loop, you should account for the possibility that fgets() returns NULL (indicating end-of-file before any input is read, or error). On the other hand, you probably do not have to account for input[0] == '\0' because fgets() always copies at least one character from input to buffer upon success (provided you specify a buffer size of at least 2). Literal '\0' characters in the input could conceivably trip you up, but if you need to accommodate those then you need to handle input altogether differently.

If you mean to accept only one command per input line, then I'd recommend consuming the balance of the line, up to the next newline, at the bottom of each iteration of the input loop. The one thing to watch out for there would be input lines containing a newline at index 1, which your current code will reject as invalid, but for which the trailing newline will already have been read.

Issues with headers and #includes

As a matter mostly of style, each of your headers should #include those headers defining constants and identifiers used directly by that header, if any, but not any other headers. (Each C source file should do the same.) Do not otherwise have your headers include other headers; it is unnecessary, and under some circumstances it can be harmful.

For example, given it's current contents, your gym_monitor.h is right to include pthread.h and semaphore.h, but there appears to be no reason for it to include stdio.h. On the other hand, I would encourage having it include main.h for the definition of struct Student, or else to combine those two headers into one.

As a separate matter, it is a good idea to ensure that each source file and header that #includes headers includes them in the same relative order. This is less important for standard library headers, but there's no good reason to distinguish. It can be the case that changing the order of headers changes their interpretation (which would be a weakness of one or more of the headers involved, but sometimes that happens). In your case, your files differ on the order of gym_monitor.h and main.h.

StackExchange Code Review Q#148688, answer score: 4