HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Automated Safety System Daemon in C89

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
c89systemautomatedsafetydaemon

Problem

Explanation

This software is currently designed to:

  • Spawn a daemon which controls and monitors multiple threads



  • Create threads based on a 'list' of functions



  • Monitor threads for operation



  • Restart threads if not running



  • Cleanly stop threads and exit upon SIGTERM



It runs on a single embedded system, operating Debian 8 and is controlled with systemd. Logging is currently through rsyslog and is filtered to a separate file (based on the program name).

The software is being developed on Debian 8 using eclipse-cdt IDE and GNU autotools.

Side note

Each thread interfaces with a modular field I/O unit (MODBUS TCP/IP), reading sensor inputs and activating controls based on these inputs.

Each I/O unit has a theoretical minimum throughput of 3.6 million 16-bit registers per second at 100BaseT (sorry, I don't know how to better quantify that). Basically this software will eventually be handling a lot of I/O.

Requests

This is the first time I have written a multi-threaded program in C. I would primarily like feedback on:

  • How the code manages memory or any memory related issues that were overlooked (buffer overflows etc)



  • Any error checking that I missed or that is redundant



  • Using boolean flags to control loops (mutexes were considered, but deemed unnecessary)



  • Thread management (creation, monitoring and destruction)



  • Logging methodology



EDIT: I know "The effects of signal() in a multithreaded process are unspecified", according to the man page for signal(). I'm currently migrating to sigaction but experiencing issues.

primary source (ascd.c)

Note: In this posting asc_main_0() is the function which runs to interface with the I/O. All that needs to be know about it is that it sets threads[i].tec to TRUE while in operation.

`/*
* ascd.c
*/

/*
* Automated Safety Control System Daemon
*/
/ Includes /
#include / libconfuse for file parsing /
#include / shared library /
#include / individual thread functions /

/* Function

Solution

Nice first question on Code Review, and definitely ambitious.

There are a definitely some good habits shown in the code, such as the guard banding in the include file and the include file including all the other header files it needs.

The code isn't quite portable. I tried to build it on Linux rather than Windows and got compilation error messages. It might be better to either write portable code, or at least name the targeted operating system in the question.

Global Variables

Many software engineers consider global variables to be evil. The use of global variables makes code harder to write, debug and maintain. It creates tight coupling between modules.

The libasc.h header file contains

/* GLOBAL variable declarations */  
prog_master PROGRAM;  
volatile bool RUN;


The previous declarations reserve memory for the globals in each object file, in essence this is creating a separate variable in each oject file.

This should be:

/* GLOBAL variable declarations */  
extern prog_master PROGRAM;  
extern volatile bool RUN;


If global variables are going to be shared between modules the header file should declare the global variables as extern because as defined above the variables are declared in each source file and this should cause multiple symbol definitions at link time (it should be reported as a link error).

Global variables should never be used in shared libraries, it creates dependencies in the executable binaries that are hard to track down.

libasc

There are functions in libasc that are surprising in a shared library,
prog_exit(), sig_break() and argv_check() are functions that one expects in the main executable binary rather than in a shared library. What would be more expected is features shared between executables such as functions that create and alter the prog_master and thread structs.

Functions that are Declared but not Defined

The code contains a static declaration for the function print_info(), but the function is never defined and never used. A good suggestion is to always compile with warnings during development, this will help to eliminate bugs.

Code Snippets

/* GLOBAL variable declarations */  
prog_master PROGRAM;  
volatile bool RUN;
/* GLOBAL variable declarations */  
extern prog_master PROGRAM;  
extern volatile bool RUN;

Context

StackExchange Code Review Q#136758, answer score: 4

Revisions (0)

No revisions yet.