Red light, green light... kinda

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
#include
#include
#include
#include
#include
#include
#include
#include

#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 /

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

StackExchange Code Review Q#147223, answer score: 3