8
\$\begingroup\$

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 <confuse.h> /* libconfuse for file parsing */
#include <libasc.h> /* shared library */
#include <thread_functions.h> /* individual thread functions */

/* Function prototypes */
static int start();
static void d_loop();
static void stop();

static int conf_parse();
static int daemonize();


int main(int argc, char *argv[]){
    /* Set program master */
    PROGRAM.name = "ascd";
    PROGRAM.pid = getpid();
    PROGRAM.info = "Automated Safety Control System Daemon\nVersion: 0.3.0.0\n\nUsage: ascd {start|stop}\n\n";
    PROGRAM.pid_file = "/var/run/ascd.pid";
    PROGRAM.conf = "/etc/asc/ascd.conf";
    PROGRAM.log_pri = LOG_NOTICE;
    PROGRAM.req_args = 2;

    /* Set default log mask */
    setlogmask(LOG_UPTO(PROGRAM.log_pri));

    /* Check for argv */
    argv_check(argc, PROGRAM.req_args);

    /* Start Daemon */
    if(strcmp(argv[1], "start") == 0){
        start();
        d_loop();
    }

    /* Stop Daemon */
    else if(strcmp(argv[1], "stop") == 0){
        stop();
        return(EXIT_SUCCESS);
    }

    /* Default */
    else {
        printf(PROGRAM.info);
        exit(EXIT_FAILURE);
    };

    /* Cleanup fs */
    if(file_exists(PROGRAM.pid_file)){
        unlink(PROGRAM.pid_file);
    };

    /* Success */
    return(EXIT_SUCCESS);
}


static int start(){
/*
 * Starts asc daemon
 *
 * Arguments:
 *  None
 *
 * Returns:
 *  int: {EXIT_SUCCESS|EXIT_FAILURE}
 */
    slog(LOG_NOTICE, "NOTICE: STARTING ASCD...\n");

    /* Parse config */
    conf_parse();

    /* Set log mask */
    slog(LOG_NOTICE, "NOTICE: start(): setting user LOG_PRI [%d]...\n", PROGRAM.log_pri);
    setlogmask(LOG_UPTO(PROGRAM.log_pri));

    /* Daemonize the program */
    if(daemonize()){
        slog(LOG_ERR, "ERROR: start(): daemonization failed\n");
        prog_exit(EXIT_FAILURE);
    };

    /* SIGNAL handling */
    slog(LOG_DEBUG, "DEBUG: start(): SIGTERM handling initiation...\n");
    signal(SIGTERM, sig_break);

    return(EXIT_SUCCESS);
}


static void d_loop(){
/*
 * The main execution loop of the daemon.
 *
 * Arguments:
 *  None
 *
 * Returns:
 *  None
 */
    slog(LOG_NOTICE, "NOTICE: ASCD SUCCESSFULLY STARTED\n");
    /* Set main loop flag */
    RUN = TRUE;

    /* Initialize cURL globally (required for multi-thread programs) */
    if(curl_global_init(CURL_GLOBAL_NOTHING)){
        slog(LOG_ERR, "ERROR: curl_global_init(): %s [%d]\n", strerror(errno), errno);
    };

    /* Variable declarations and definitions */
    int i;
    size_t poll_time = 30;
    size_t num_thread = 1;

    /* Declare threads in array */
    struct thread threads[num_thread];

    /* Define functions to execute */
    threads[0].name = "asc_main_0";
    threads[0].function = (intptr_t) asc_main_0;
/*  threads[1].name = "example_fn";
    threads[1].function = (intptr_t) example_fn;*/

    /* Trigger thread run flag(s) and zero tec */
    for(i=0;i<num_thread;i++){
        threads[i].run = TRUE;
        threads[i].tec = FALSE;
    };

    /* Daemon-specific initialization */
    while(RUN){
        /* Create all threads */
        for(i=0;i<num_thread;i++){
            slog(LOG_INFO, "INFO: dloop(): checking for thread %s's existence...\n", threads[i].name);
            /* Thread does not exist */
            if(!threads[i].tec){
                slog(LOG_INFO, "INFO: dloop(): thread %s does not exist\n", threads[i].name);

                /* Create thread */
                slog(LOG_INFO, "INFO: dloop(): creating thread %s...\n", threads[i].name);
                if(pthread_create(&(threads[i].thread), NULL, (void*) threads[i].function, &threads[i])){
                    slog(LOG_ERR, "ERROR: dloop(): cannot create thread %s: %s [%d]\n", threads[i].name, strerror(errno), errno);

                };
            } else {
                /* Thread exists */
                slog(LOG_INFO, "INFO: dloop(): thread %s exists [%d]\n", threads[i].name, threads[i].tid);
            };
        };

        /* Wait to re-check threads */
        slog(LOG_INFO, "INFO: dloop(): sleeping for %d seconds...\n", poll_time);
        sleep(poll_time);
    };

    /* Wait for threads to stop */
    for(i=0;i<num_thread;i++){
        /* Trigger threads to terminate */
        while(threads[i].tec){
            threads[i].run = FALSE;
            slog(LOG_INFO, "INFO: dloop(): waiting for thread %s to stop...\n", threads[i].name);
            sleep(5);
        };
    };

    /* Cleanup and exit program */
    curl_global_cleanup();
    slog(LOG_NOTICE, "NOTICE: ASCD SUCCESSFULLY STOPPED\n");
}


static void stop(){
/*
 * Stops asc daemon
 *
 * Arguments:
 *  None
 *
 * Returns:
 *  None
 */

    /* Declare variables */
    FILE *f;
    int pid = 0;

    /* Open and read PID file */
    f = fopen(PROGRAM.pid_file, "r");
    /* Check if program is running */
    if(f){
        fscanf(f, "%d", &pid);

        /* Send SIGTERM to daemon */
        slog(LOG_NOTICE, "NOTICE: SIGNALING ASCD TO STOP [%d]...\n", pid);
        if(kill(pid, SIGTERM)){
            slog(LOG_ERR, "ERROR: kill(): %s [%d]\n", strerror(errno), errno);
        };

        /* Close file */
        fclose(f);
    };
}


static int conf_parse(){
/*
 * Uses libconfuse to parse configuration files stored in /etc.
 *
 * Arguments:
 *  int log_pri: pointer address for global variable
 *
 * Returns:
 *  int: {EXIT_SUCCESS|EXIT_FAILURE}
 *
 * Sources:
 *  https://github.com/martinh/libconfuse/blob/master/examples/cfgtest.c
 */
    slog(LOG_INFO, "INFO: conf_parse(): parsing config...\n");
    /* Declare variables */
    cfg_opt_t opts[] = {
            CFG_INT("LOG_PRI", 4, CFGF_NONE),
            CFG_END()
    };
    cfg_t * cfg;

    /* Define configuration file */
    slog(LOG_DEBUG, "DEBUG: conf_parse(): config file definition\n");
    cfg = cfg_init(opts, CFGF_NONE);
    if(cfg_parse(cfg, PROGRAM.conf) == CFG_PARSE_ERROR){
        slog(LOG_ERR, "ERROR: conf_parse(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    }

    /* Assign global variable values */
    slog(LOG_DEBUG, "DEBUG: conf_parse(): Parsing config file\n");
    PROGRAM.log_pri = cfg_getint(cfg, "LOG_PRI");

    /* Return variable struct */
    slog(LOG_INFO, "INFO: conf_parse(): success\n");
    return(EXIT_SUCCESS);
}


static int daemonize(){
/*
 * Creates a child process from parent (daemon) and kills the parent.
 *
 * Arguments:
 *  None
 *
 * Returns:
 *  int: {EXIT_SUCCESS|EXIT_FAILURE}
 *
 * Methodology:
 *  Fork the Parent process, creating Child, and exit Parent
 *  Create unique Session containing a single (new) process group
 *  Child becomes session and process group leader (independent)
 *  Write PID to lock file
 *  Change the current working directory to daemon's directory
 *  Close inherited descriptors and standard I/O descriptors
 *  Reopen stdin, stdout, stderr
 *  Change file mode mask (umask)
 *
 * Sources:
 *  http://codingfreak.blogspot.com/2012/03/daemon-izing-process-in-linux.html
 *  http://web.archive.org/web/20120914180018/http://www.steve.org.uk/Reference/Unix/faq_2.html#SEC16
 *  http://www.netzmafia.de/skripten/unix/linux-daemon-howto.html
 *  https://github.com/jirihnidek/daemon/blob/master/src/daemon.c
 */
    slog(LOG_INFO, "INFO: daemonize(): daemonizing...\n");

    /* Declare variables */
    pid_t frtn;
    static char * ddir = "/tmp";

    /* Fork the Parent process, creating child, and exit Parent */
    slog(LOG_DEBUG, "DEBUG: daemonize(): forking process...\n");
    frtn = fork();

        /* Fork error occurred */
        if(frtn < 0){
            slog(LOG_ERR, "ERROR: daemonize(): fork failure [%d]\n", frtn);
            return(EXIT_FAILURE);
        } else

        /* Fork success, parent terminates */
        if(frtn > 0){
            slog(LOG_INFO, "INFO: daemonize(): child created [%d], terminating parent [%d]\n", frtn, PROGRAM.pid);
            exit(EXIT_SUCCESS);
        };

    /* Child continues from here */
    /* Create unique session containing a single (new) process group */
        slog(LOG_DEBUG, "DEBUG: daemonize(): creating unique session...\n");
    if(setsid() < 0){
        /* setssid failure, returned -1 */
        slog(LOG_ERR, "ERROR: daemonize(): setsid(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    };

    /* Update PID */
    PROGRAM.pid = getpid();

    /* Success, child becomes session and process group leader (independent) */
    slog(LOG_INFO, "INFO: daemonize(): successfully started ascd [%d]\n", PROGRAM.pid);

    /* Ignore signal(s) sent from child to parent */
    slog(LOG_DEBUG, "DEBUG: daemonize(): ignoring SIGCHLD and SIG_IGN...\n");
    signal(SIGCHLD, SIG_IGN);

    /* Check for pid_file */
    if(!PROGRAM.pid_file){
        slog(LOG_WARNING, "WARNING: daemonize(): pid_file is null, no lock file created\n");
        return(EXIT_FAILURE);
    };

    /* Write PID to lock file */
    slog(LOG_INFO, "INFO: daemonize(): writing PID [%d] to lockfile [%s]...\n", PROGRAM.pid, PROGRAM.pid_file);

    /* Variable declaration */
    static char str[256];
    static int pid_fd = -1;

    slog(LOG_DEBUG, "DEBUG: daemonize(): opening lock file...\n");
    pid_fd = open(PROGRAM.pid_file, O_RDWR|O_CREAT, 0640);

    /* Checks */
    if(pid_fd < 0){
        /* Can't open lock file */
        slog(LOG_ERR, "ERROR: daemonize(): open(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);

    };

    slog(LOG_DEBUG, "DEBUG: daemonize(): locking file...\n");
    if(lockf(pid_fd, F_TLOCK, 0) < 0){
        /* Can't lock file */
        slog(LOG_ERR, "ERROR: daemonize(): lockf(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    };

    /* Get current PID */
    sprintf(str, "%d", PROGRAM.pid);

    /* Write PID to lock file */
    slog(LOG_DEBUG, "DEBUG: daemonize(): writing %s to %s...\n", str, PROGRAM.pid_file);
    if(write(pid_fd, str, strlen(str)) == -1){
        slog(LOG_ERR, "ERROR: daemonize(): write(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    };

    /* Change working directory to daemon's */
    slog(LOG_DEBUG, "DEBUG: daemonize(): changing working directory to %s\n", ddir);
    if(chdir(ddir)){
        slog(LOG_ERR, "ERROR: daemonize(): chdir(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    };

    /* Close inherited descriptors and standard I/O descriptors */
    slog(LOG_DEBUG, "DEBUG: daemonize(): closing file descriptors\n");
    int i;
    for(i = getdtablesize();i>=0; --i){
        close(i);
    };

    /* Reopen stdin, stdout, stderr */
    slog(LOG_DEBUG, "DEBUG: daemonize(): opening file descriptors\n");
    stdin = fopen("/dev/null", "r"); /* fd = 0 */
    stdout = fopen("/dev/null", "w+"); /* fd = 1 */
    stderr = fopen("/dev/null", "w+"); /* fd = 2 */

    if(stdin < 0 || stdout < 0 || stderr < 0){
        slog(LOG_ERR, "ERROR: daemonize(): open(): %s [%d]\n", strerror(errno), errno);
        return(EXIT_FAILURE);
    };

    /* Change file mode mask (umask) */
    slog(LOG_DEBUG, "DEBUG: daemonize(): changing file mode mask\n");
    umask(027); /* mode 750 */

    slog(LOG_INFO, "INFO: daemonize(): success\n");
    return(EXIT_SUCCESS);
}

shared library header (libasc.h)

/*
 * libasc.h
 */

#ifndef libasc_H
#define libasc_H

/* Includes */
#include <curl/curl.h>
#include <errno.h>
#include <fcntl.h>
#include <pthread.h>
#include <signal.h>
#include <syslog.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <time.h>
#include <unistd.h>

/* Default syslog facility */
#define DFACILITY LOG_DAEMON

/* Returns name of variable */
#define getName(var) #var

/* Program information */
typedef struct prog_master {
    char *name;
    pid_t pid;
    char *info;
    char *pid_file;
    char *conf;
    int log_pri;
    size_t req_args;
} prog_master;

/* Individual thread */
typedef struct thread {
    char *name;
    intptr_t function;
    pthread_t thread;
    pid_t tid;
    volatile bool run; /* controls thread loop */
    volatile bool tec; /* thread existence check */
    void *exit_val;
} thread;


/* Function prototypes */
void sig_break(int sig);
int file_exists(const char *fname);
void slog(int priority, char *msg, ...);
void argv_check(int ac, int nm);
void prog_exit(int code);    

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

#endif

shared library (libasc.c)

/*
 * libasc.c
 */

#include <libasc.h>

void sig_break(int sig){
/*
 * Catches SIG and breaks main loop
 * Signals should probably not be used in child threads
 * Instead have the main thread handle them
 *
 * Arguments:
 *  int sig: SIGNAL received
 *
 * Returns:
 *  None
 *
 * Sources:
 *  https://www.gnu.org/software/libc/manual/html_mono/libc.html#Handler-Returns
 *  http://www.thegeekstuff.com/2012/03/catch-signals-sample-c-code/
 */
    slog(LOG_NOTICE, "NOTICE: SIGNAL %d RECEIVED\n", sig);
    if( sig == SIGTERM){
        RUN = 0; /* clear main loop flag */
    };
}


int file_exists(const char *fname){
/*
 * Checks for file existence
 *
 * Arguments:
 *  char *fname: absolute filename
 *
 * Returns:
 *  int: 0 if file exists
 */
    struct stat buffer;
    return(stat(fname, &buffer) == 0);
}


void slog(int priority, char *msg, ...){
/*
 * Writes to syslog with the default (given in program) facility
 *
 * Arguments:
 *  int priority: syslog message priority
 *  char *msg: Typical syslog message (format)
 *  ...: Additional variables
 *
 * Returns:
 *  None
 */
    /* Declare variables */
    va_list list;

    /* Create va_list from ... */
    va_start(list, msg);

    /* open, write and close log */
    openlog(PROGRAM.name, LOG_CONS|LOG_PID|LOG_NDELAY, DFACILITY);
    vsyslog(priority, msg, list);
    closelog();

    va_end(list);
}

void argv_check(int ac, int nm){
/*
 * Checks for the required number of arguments
 *
 * Arguments:
 *  int ac: number supplied (usually given through argc)
 *  int nm: number required
 *
 * Returns:
 *  int: 0 if ac < nm, 1 else
 */
    if(ac < nm){
        slog(LOG_WARNING, "WARNING: main(): no arguments giving...exiting\n");
        printf(PROGRAM.info);
        exit(EXIT_FAILURE);
    };
}

void prog_exit(int code){
/*
 * Exits the program cleanly
 * Only use once it has entered d_loop()
 *
 * Arguments:
 *  int code: exit code
 *
 * Returns:
 *  None
 */
    curl_global_cleanup();
    exit(code);
}
\$\endgroup\$
8
  • 2
    \$\begingroup\$ Nice question. I hope you get good reviews. \$\endgroup\$
    – Heslacher
    Aug 3, 2016 at 15:24
  • 4
    \$\begingroup\$ Why C89? That's a 27 year old standard you're developing for, you should at least be using C99 IMO. \$\endgroup\$
    – syb0rg
    Aug 3, 2016 at 15:47
  • 2
    \$\begingroup\$ @syb0rg Good Question. Honestly, didn't give it much thought until now. After looking over the differences, I'm practically using C99 already. I'll look into migrating over. \$\endgroup\$
    – Nick
    Aug 3, 2016 at 16:02
  • 1
    \$\begingroup\$ Is function prototyping allowed in C89 as inint main(int argc, char *argv[]) - I have doubts? It have been along time. \$\endgroup\$ Aug 3, 2016 at 23:43
  • 1
    \$\begingroup\$ Where is the code for asc_main_0()? Who sets thread[i].tec to true? \$\endgroup\$
    – JS1
    Aug 4, 2016 at 1:55

1 Answer 1

4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Great response, thank you. I should clarify on 'shared library'. I call it that because the functions included there are also used in various test programs I have built within the same project. If it wasn't for those test programs, I would have included those functions in the main binary. \$\endgroup\$
    – Nick
    Aug 3, 2016 at 18:04
  • \$\begingroup\$ And that is weird you received compilation errors, I am building it on debian 8 with gcc. If you could post those errors in your response it would be appreciated. \$\endgroup\$
    – Nick
    Aug 3, 2016 at 18:07
  • 1
    \$\begingroup\$ @Nick I'm compiling using gcc on CentOS 7. \$\endgroup\$
    – pacmaninbw
    Aug 3, 2016 at 18:16
  • \$\begingroup\$ @Nick would you care to join me in the 2nd monitor chat room so we can discuss the error I get? \$\endgroup\$
    – pacmaninbw
    Aug 3, 2016 at 18:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.