patterncMinor
Automated Safety System Daemon in C89
Viewed 0 times
c89systemautomatedsafetydaemon
Problem
Explanation
This software is currently designed to:
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:
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
`/*
* ascd.c
*/
/*
* Automated Safety Control System Daemon
*/
/ Includes /
#include / libconfuse for file parsing /
#include / shared library /
#include / individual thread functions /
/* Function
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
The libasc.h header file contains
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:
If global variables are going to be shared between modules the header file should declare the global variables as
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,
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.
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.