Red light, green light… kinda

Question

For an assignment I had to create a client and a server that communicate over a well known FIFO. The server is required to use three threads to serve the client, managed by a semaphore.

The code was pretty hastily written due to my busy schedule, but works as intended from what testing I've done. Feel free to rip it apart.

---

info.h:

#ifndef INFO_H
#define INFO_H

#define SERVER_FILENAME "fifo.dat"
#define CLIENT_FILENAME "back.dat"
#define MAX_MSG_SIZE 15

#endif // INFO_H

server.c:

#include <fcntl.h>
#include <pthread.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <string.h>
#include <semaphore.h>
#include <unistd.h>

#include "info.h"

void cleanup(void)
{
    unlink(SERVER_FILENAME);
    unlink(CLIENT_FILENAME);
}

void listener(void *sem)
{
    char buf[BUFSIZ] = "";
    int err = 0;

    while (true)
    {
        sem_wait(sem);
        /* open, read, and display the message from the FIFO */
        int client_to_server = open(SERVER_FILENAME, O_RDONLY);
        int server_to_client = open(CLIENT_FILENAME, O_WRONLY);

        if (client_to_server < 0 || server_to_client < 0)
        {
            fputs("Unable to open FIFOs for reading\n", stderr);
            cleanup();
            return;
        }
        err = read(client_to_server, buf, BUFSIZ);
        if (err < 0)
        {
            fputs("Unable to read from FIFO\n", stderr);
            cleanup();
            return;
        }

        // close for reopening with new client
        close(client_to_server);

        if (strcmp("exit",buf) == 0)
        {
            printf("Server OFF.\n");
            write(server_to_client, "killed", strlen("killed"));
            close(server_to_client);
            cleanup();
            exit(0);
        }
        else
        {
            printf("Received: %s\n", buf);
            printf("Sending back...\n");
            char *new = strcat(buf, "-check");
            write(server_to_client, new, strlen(new));
        }
        close(server_to_client);

        /* clean buf from any data */
        memset(buf, 0, sizeof(buf));
        sem_post(sem);
    }
    cleanup();
}

int main(void)
{
    int err = 0;
    char *myfifo = SERVER_FILENAME;
    char *myfifo2 = CLIENT_FILENAME;

    /* create the FIFO (named pipe) */
    err = mkfifo(myfifo, 0666);
    if (err < 0)
    {
        fputs("Error creating FIFO\n", stderr);
        cleanup();
        return 1;
    }
    err = mkfifo(myfifo2, 0666);
    if (err < 0)
    {
        fputs("Error creating FIFO\n", stderr);
        cleanup();
        return 2;
    }

    printf("Server ON.\n");

    // unlink then initialize semaphore to 1 so locking works
    sem_unlink("mysem");
    sem_t *sem = sem_open("mysem", O_CREAT | O_EXCL, S_IRUSR | S_IWUSR, 1);
    if (sem == SEM_FAILED)
    {
        fputs("Unable to initialize semaphore\n", stderr);
        cleanup();
        return 3;
    }

    // initialize threads
    int num_threads = 3;
    pthread_t *tid = malloc(num_threads * sizeof(pthread_t));

    for(int i = 0; i < num_threads; ++i)
    {
        err = pthread_create(tid + i, NULL, (void*)listener, sem);
        if (err)
        {
            fputs("Unable to create threads\n", stderr);
            cleanup();
            return 4;
        }
    }
    for(int i = 0; i < num_threads; ++i)
    {
        err = pthread_join(tid[i], NULL);
        if (err)
        {
            fputs("Unable to join threads\n", stderr);
            cleanup();
            return 5;
        }
    }
    cleanup();
}

client.c:

#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#include "info.h"

int main(void)
{
    char *myfifo = SERVER_FILENAME;
    char *myfifo2 = CLIENT_FILENAME;

    char str[BUFSIZ] = "";
    char rec[BUFSIZ] = "";
    printf("Input message to server: ");
    scanf("%s", str);


    /* write str to the FIFO */
    int client_to_server = open(myfifo, O_WRONLY);
    int server_to_client = open(myfifo2, O_RDONLY);
    write(client_to_server, str, sizeof(str));

    read(server_to_client, rec, sizeof(rec));

    printf("...received from the server: %s\n", rec);
    close(client_to_server);
    close(server_to_client);
}

Answer 1

A couple of small points first...

• MAX_MSG_SIZE is defined in your header file, but not actually used in the code.
• In your client you do this char *myfifo2 = CLIENT_FILENAME and then use the local variable for opening the FIFO. All this does is obscure what it is that myfifo2 represents. It would have been better to just use CLIENT_FILENAME in the open call.

I think you've missed the point

The code that you've presented isn't what I would have been expecting given the assignment that you've described (I'm assuming there's probably actually more detail to the assignment than you've included). Stepping back from the code as written, you've been asked to write a server that uses multiple threads to service a client. The threads should be managed by a semaphore.

Threading has a cost, so based on that description I'd be asking why do I need multiple threads to handle a single client. The reason that springs to mind is that the client can supply multiple requests to the server, which should be able to handle multiple requests concurrently. For this to be worthwhile the server needs to do something that takes some time to process (otherwise you might as well be handling it on a single thread).

Note, the code supplied below isn't production quality, I've taken short cuts by making variables global and skipped error checking, it also hasn't been extensively tested but should illustrate my points.

client.c

Firstly, I'd have changed your client so that instead of sending a single message it was capable of sending multiple messages and processing the responses asynchronously. To do that in the code below I've created a second thread. So I have one thread responsible for reading from the console and writing to the FIFO and a second thread responsible for reading from the FIFO and writing to the console.

This gives the following basic client:

#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <pthread.h>
#include <string.h>
#include <stdbool.h>

#include "info.h"

int client_to_server; 

pthread_t thread;

void input_reader(void*unused)
{
    char str[BUFSIZ]="";
    while(true) {
        scanf("%s", str);
        write(client_to_server, str, MAX_MSG_SIZE);
        if(strcmp("exit", str) == 0) {
           return;
        }
    };
}

int main(void)
{
    char *myfifo = SERVER_FILENAME;
    char *myfifo2 = CLIENT_FILENAME;

    char str[BUFSIZ] = "";
    char rec[BUFSIZ] = "";
    printf("Input message to server: ");
    scanf("%s", str);


    client_to_server = open(myfifo, O_WRONLY);
    int server_to_client = open(myfifo2, O_RDONLY);

    pthread_create(&thread, NULL, (void*)input_reader, NULL);

    while(true) {
        read(server_to_client, rec, sizeof(rec));

        printf("...received from the server: %s\n", rec);
        if(strstr(rec, "killed")) break;
    }

    pthread_join(thread, NULL);

    close(client_to_server);
    close(server_to_client);
}

server.c

For the server:

• I've introduced a simulated long running task (essentially I've put a 3 second sleep in between starting to process a message and sending the response).
• I've changed the responsibility of the threads. The '3 created threads' are responsible for processing messages and sending responses to the client. The main thread is responsible for reading input from the client and queuing them up for processing.
• I've changed the semaphore usage to represent the number of items waiting to be processed. As messages are received from the client, the reader thread increments the semaphore which releases a worker thread to process the request. If messages are received faster then they are processed then multiple worker threads will become active (up to the maximum 3). They will stay active until the backlog is cleared.
• I've created an internal queue for handling the messages to process. It may be possible to avoid the need for this and use the FIFO directly, however I have little experience with FIFO's, but having multiple threads read from FIFO with my brief testing seems unreliable.
• I've introduced two mutexes to protect resources from multiple threads. The first qlock is used to protect the internal queue when putting work item on/taking them off the queue. The second wlock is used to protect the FIFO when writing to the client (this might not be necessary since the OS will probably do this anyway).
• The code supplied has some known issues. I'm not handling shutdown which means that it can crash when exit is sent/processed. The first message written to the FIFO by the client is ignored. Sometimes the server fails to create the FIFO, although this is resolved after the cleanup is called so it starts when called again.

Which gives the following server code:

#include <fcntl.h>
#include <pthread.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <string.h>
#include <semaphore.h>
#include <unistd.h>

#include "info.h"

void cleanup(void)
{
    unlink(SERVER_FILENAME);
    unlink(CLIENT_FILENAME);
}

typedef struct node {
    struct node *next;
    char buffer[BUFSIZ];
}NODE;

NODE *head = NULL;
NODE *tail = NULL;
pthread_mutex_t qlock;
pthread_mutex_t wlock;

void enqueue(char *message) {
  NODE *node = malloc(sizeof(node));
  strcpy(node->buffer, message);

  pthread_mutex_lock(&qlock);
  if(!tail)
  {
    head = tail = node;
  }
  else
  {
    tail->next = node;
    tail = node;
  }
  pthread_mutex_unlock(&qlock);
}

void dequeue(char *destination) {
  pthread_mutex_lock(&qlock);
  NODE *current = head;
  head = head->next;
  if(head == NULL) tail = NULL;
  pthread_mutex_unlock(&qlock);
  strcpy(destination, current->buffer);
  free(current);
}

void readloop(void *sem)
{
    char buf[BUFSIZ] = "";
    int err = 0;

    int client_to_server = open(SERVER_FILENAME, O_RDONLY);

    if (client_to_server < 0)
    {
        fputs("Unable to open FIFOs for reading\n", stderr);
        cleanup();
        return;
    }

    while(true) {
        err = read(client_to_server, buf, BUFSIZ);
        if (err < 0)
        {
            fputs("Unable to read from FIFO\n", stderr);
            cleanup();
            return;
        }

        enqueue(buf);
        sem_post(sem);
        memset(buf, 0, sizeof(buf));
    }
}

void listener(void *sem) {
    char buf[BUFSIZ]="";

    int server_to_client = open(CLIENT_FILENAME, O_WRONLY);

    while(true) {
        sem_wait(sem);

        dequeue(buf);

        sleep(3);  // Simulate long running task

        pthread_mutex_lock(&wlock);
        if (strcmp("exit",buf) == 0)
        {
            printf("Server OFF.\n");
            write(server_to_client, "killed", strlen("killed"));
            close(server_to_client);
            cleanup();
            exit(0);
        }
        else
        {
            printf("Received: %s\n", buf);
            printf("Sending back...\n");
            char *new = strcat(buf, "-check");
            write(server_to_client, new, strlen(new));
        }
        pthread_mutex_unlock(&wlock);

        memset(buf, 0, sizeof(buf));
    }
    cleanup();
}

int main(void)
{
    int err = 0;
    char *myfifo = SERVER_FILENAME;
    char *myfifo2 = CLIENT_FILENAME;

    /* create the FIFO (named pipe) */
    err = mkfifo(myfifo, 0666);
    if (err < 0)
    {
        fputs("Error creating FIFO\n", stderr);
        cleanup();
        return 1;
    }
    err = mkfifo(myfifo2, 0666);
    if (err < 0)
    {
        fputs("Error creating FIFO\n", stderr);
        cleanup();
        return 2;
    }

    printf("Server ON.\n");

    // unlink then initialize semaphore to 1 so locking works
    sem_unlink("mysem");
    sem_t *sem = sem_open("mysem", O_CREAT | O_EXCL, S_IRUSR | S_IWUSR, 0);
    if (sem == SEM_FAILED)
    {
        fputs("Unable to initialize semaphore\n", stderr);
        cleanup();
        return 3;
    }

    pthread_mutex_init(&qlock, NULL);
    pthread_mutex_init(&wlock, NULL);

    int num_threads = 3;
    pthread_t *tid = malloc(num_threads * sizeof(pthread_t));

    for(int i = 0; i < num_threads; ++i)
    {
        err = pthread_create(tid + i, NULL, (void*)listener, sem);
        if (err)
        {
            fputs("Unable to create threads\n", stderr);
            cleanup();
            return 4;
        }
     }

    readloop(sem);

    for(int i = 0; i < num_threads; ++i)
    {
        err = pthread_join(tid[i], NULL);
        if (err)
        {
            fputs("Unable to join threads\n", stderr);
            cleanup();
            return 5;
        }
    }
    cleanup();
}

One more thing

Processing concurrent client messages also introduces a question of whether or not synchronisation is important. It's possible that the third message received might be processed faster than the second message, resulting in it being sent back to the client before the second. If ordering of the messages is important then either the server would need to manage this, or a message structure would need to be passed (for example that contained a message id) rather than just the text.

Answer 2

before calling mkfifo() suggest checking of the fifo already exists. OR after calling mkfifo(), and it returns -1, check errno for the value EEXIST as that is not a problem in the current scenario.

regarding this line:

scanf("%s", str); 

1) always check the returned value (not the parameter values) to assure the operation was successful.

2) when using the %s format specifier, always include a MAX CHAR modifier that is one less than the input buffer length, so the user cannot overrun the input buffer (which would result in undefined behavior and can lead to a seg fault event.

BUFSIZ is not defined in the posted code

regarding the call to read() and the following call to printf(): keep the returned value from the call to read() and use that to NUL terminate the char array rec[] as the call to read() will not do that for you.

this line, in the server:

sem_wait(sem); 

will not cleanly compile as the call to sem_wait() is expecting a pointer to sem_t suggest casting as in:

sem_wait( (sem_t*)sem);

in the server, after each call to open(), immediately check if returned value <0 and if so, then call perror() so the reason for the failure to open the fifo is also output. Even better would be to call syslog() so there is a long term record of the error

in the client, after each call to open(), immediately check if the returned value is <0 and if so, then call perror(), so the reason for the failur to open the fifo is also output.

regarding this (and following lines)

err = read(client_to_server, buf, BUFSIZ);

the function: read() will not NUL terminate the read in char array, so use the value in err to terminate the string, similar to:

buf[err] = '\0';

otherwise the calls to strcmp() will fail

When exiting a thread, do not just run off the end of the thread, rather use something similar to:

int status = 0;
pthread_exit( &status );

the proper prototype for a thread has a return type of void* not void So the return statements in the 'listener' thread should be calls to pthread_exit() and the signature needs to be corrected

the threads should not call cleanup(), nor should they exit the program, They should always call pthread_exit() and let the main function call cleanup().

The listener() thread is exiting with the semaphore locked (due to the call to sem_wait() before exiting. The thread should always call sem_post() no matter what reason is causing the exit of the thread.

the variable new is a very poor choice for a variable name because that is a reserved word in the C++ language and if this program is compiled with a compiler that can also be used for C++ then it could cause confusion

the cleanup() function should check that a fifo exists before calling unlink() so as to not cause confusion nor a possible program abort