5
\$\begingroup\$

This question is a follow-up to a previous version of this program, which I wrote because I found it annoying that VLC Media Player (which I love) prevents the screensaver from starting after playback of media has ended on my Ubuntu 12.04 system.

There have been several changes which are listed in the comments at the top of the code.

I'm not trying to play code golf, but I'm looking for ways to shorten the code or do things more efficiently. General suggestions for any other improvements or ways to make this code more portable are welcome.

/* VLC-Watchdog v6 
 *   by Jeremy B. Beck
 *
 * A fix for stopped VLC Media Player inhibiting the power management
 * daemon and preventing screen saver and/or monitor power off on my Ubuntu
 * 12.04 system using the DBus low level API.
 *
 * Takes start | stop as command line argument.
 *
 * Checks if VLC Media Player is running by searching for the interface in
 * the DBus list of interface names.  If VLC is found to be running, 
 * queries the playback status through the DBus Get method.  If the 
 * playback status is found to be stopped, calls the Quit() method via 
 * DBus.  If VLC is found not to be running, VLC-Watchdog sleeps for 30 
 * seconds then repeats all of the above.  When VLC-Watchdog receives
 * the stop signal (eg. "VLC-Watchdog stop" from the command line) it exits.
 * 
 * Changed in v6:
 * + Check if VLC is running by looking for the interface name via DBus 
 *   instead of using pgrep.
 * + Cleaned up code to connect to session bus.  Connect to bus in main()
 *   and pass the connection pointer to the methods that need it instead of
 *   creating a new connection in every method.
 *
 * Changed in v5:
 * + double check if VLC is stopped after 10 sec delay to prevent quitting
 *   VLC while it is starting up or loading media
 * Changed in v4:
 * + cleaned up error logging
 * + time stamped error log entries
 */

#include <stdio.h>
#include <stdlib.h>
#include <dbus/dbus.h>
#include <unistd.h>
#include <string.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <time.h>

static void skeleton_daemon();
int vlc_running(DBusConnection *conn);
int vlc_stopped(DBusConnection *conn, int pid);
int quit_vlc(DBusConnection *conn, int pid);
void usage(char *pname);
void sig_handler(int signo);
void log_error(const char *format, ...);

extern int running = 1;
extern int err_count = 0;

int main(int argc, char *argv[]) {
    FILE *fp;
    int pid, status;
    DBusConnection *conn;
    DBusError err;
    //check command line arguments for validity
    if ((argc != 2) || (
        (strcmp(argv[1], "start") != 0) && 
        (strcmp(argv[1], "stop") != 0)))
        usage(argv[0]);
    //start / run actions
    if (strcmp(argv[1], "start") == 0) {
        skeleton_daemon(); //daemonize
        //check if DBus is loaded
        if (getenv("DBUS_SESSION_BUS_ADDRESS") == NULL) {
            log_error("DBus session bus address == NULL\n");
            log_error("stopping VLC-Watchdog\n");
            exit(1);
        }
        //write the PID to a file for future reference
        fp = fopen("/tmp/vlcwatchdog.pid", "w");
        fprintf(fp, "%d\n", getpid());
        fclose(fp);
        //register signal handler
        if (signal(SIGUSR1, sig_handler) == SIG_ERR) {
            log_error("error while setting signal handler\n");
            exit(1);
        }
        /* when first started and every 30 seconds thereafter, check if
         * VLC is running, check if it is stopped, ask it to Quit() if it
         * is stopped.  Stop and exit when SIGUSR1 is received */
        dbus_error_init(&err); // init DBus error
        conn = dbus_bus_get(DBUS_BUS_SESSION, &err); //connect to session bus
        if (conn == NULL) {
            log_error(err.message);
            return 1;
        }
        while (running) {
            if ((pid = vlc_running(conn))) {
            /* if VLC is stopped wait 10 sec then check if it is still
             * stopped in an attempt to prevent quitting VLC while it is
             * starting up or loading media */
                if (vlc_stopped(conn, pid)) {
                    sleep(10);
                    if (vlc_stopped(conn, pid)) {
                        if((status = quit_vlc(conn, pid)) == 1) {
                            log_error("quit_vlc() returned 1. exiting\n");
                            break;
                        }
                    }
                }
            }
            if (err_count >= 10) {
                log_error("error count exceeded.  exiting\n");
                break;
            }
            sleep(30);
        }
        //remove temporary file
        if (remove("/tmp/vlcwatchdog.pid") == -1) {
            log_error("error removing temporary file /tmp/vlcwatchdog.pid");
            exit(1);
        }
    }
    //stop actions
    if (strcmp(argv[1], "stop") == 0) {
        //retrieve PID from file
        if (access("/tmp/vlcwatchdog.pid", R_OK) != -1) {
            fp = fopen("/tmp/vlcwatchdog.pid", "r");
            if (fscanf(fp, "%d", &pid) != 1) {
                log_error("fscanf failed to read PID from file while stopping\n");
            }
            fclose(fp);
        } 
        //if unable to read PID from file, try pgrep
        else {
            char whatsmyname[256], s[32];
            sprintf(whatsmyname, "%s%s", "pgrep ", argv[0]);
            fp = popen(whatsmyname, "r");
            if (fp != NULL) {
                if (fgets(s, 32, fp) != NULL) {
                    sscanf(s, "%d", &pid);
                    fclose(fp);
                } 
                else {
                    fclose(fp);
                    fprintf(stderr, "daemon does not appear to be running\n");
                    exit(1);
                }
            }
            else {
                fclose(fp);
                log_error("popen failed while stopping\n");
                exit(1);
            }
        }
        //issue stop signal to VLC-Watchdog daemon
        kill(pid, SIGUSR1);
    }
    exit(0);
}

/* deamonize the process */
static void skeleton_daemon() {
    pid_t pid;

    /* Fork off the parent process */
    pid = fork();

    /* An error occurred */
    if (pid < 0)
        exit(EXIT_FAILURE);

    /* Success: Let the parent terminate */
    if (pid > 0)
        exit(EXIT_SUCCESS);

    /* On success: The child process becomes session leader */
    if (setsid() < 0)
        exit(EXIT_FAILURE);

    /* Fork off for the second time*/
    pid = fork();

    /* An error occurred */
    if (pid < 0)
        exit(EXIT_FAILURE);

    /* Success: Let the parent terminate */
    if (pid > 0)
        exit(EXIT_SUCCESS);

    /* Set new file permissions */
    umask(0);

    /* Change the working directory to the root directory */
    /* or another appropriated directory */
    if (chdir("/") != 0) {
        log_error("chdir failed in skeleton_daemon\n");
        exit(1);
    }

    /* Close all open file descriptors */
    int x;
    for (x = sysconf(_SC_OPEN_MAX); x>0; x--)
    {
        close (x);
    }

}

/* check if VLC is stopped, if so, return 1, return 0 otherwise */
int vlc_stopped(DBusConnection *conn, int pid) {
    DBusError err;
    DBusMessage *method_call, *reply;
    DBusMessageIter iter, sub_iter;
    int arg_type = 0;
    const char *prop_iface = "org.mpris.MediaPlayer2.Player";
    const char *prop_iface_method_name = "PlaybackStatus";
    const char *pb_stat;
    char name[80];

    //append the pid to the name of the destination
    sprintf (name, "%s%d", "org.mpris.MediaPlayer2.vlc-", pid);
    dbus_error_init(&err); // init DBus error
    //create the method call to be used to get playback status of VLC
    method_call = dbus_message_new_method_call(name,
    "/org/mpris/MediaPlayer2", "org.freedesktop.DBus.Properties",
    "Get");
    if (method_call == NULL) {
        log_error("Get() method call is NULL after creation\n");
        return 1;
    }
    //put the method arguments on the message
    if (!dbus_message_append_args(method_call, DBUS_TYPE_STRING, 
        &prop_iface, DBUS_TYPE_STRING, &prop_iface_method_name, 
        DBUS_TYPE_INVALID)) {
        log_error("failed to append args to message\n");
        return 1;
    }
    //call the method, get the reply, check for error
    reply = dbus_connection_send_with_reply_and_block (conn, 
    method_call, -1, &err);
    //this error can be recovered from
    if (reply == NULL) {
        log_error("null reply \n %s \n", err.message);
        dbus_error_free(&err);
        err_count++;
        return 0;
    }
    //initialize an iterator pointing to arguments in reply message
    if (!dbus_message_iter_init (reply, &iter)) {
        log_error("reply message has no arguments\n");
        return 1;
    }
    //check the type of the first argument (should be variant)
    arg_type = dbus_message_iter_get_arg_type(&iter);
    if (arg_type != DBUS_TYPE_VARIANT) {
        log_error("Expected variant, got %d\n", arg_type);
        return 1;
    }
    //recurse into the variant container to get an iter to the string
    dbus_message_iter_recurse(&iter, &sub_iter);
    //check the type of the value in the variant (should be string)
    arg_type = dbus_message_iter_get_arg_type(&sub_iter);
    if (arg_type != DBUS_TYPE_STRING) {
        log_error("Expected string, got %d\n", arg_type);
        return 1;
    }
    //extract the value of the string from sub_iter
    dbus_message_iter_get_basic(&sub_iter, &pb_stat);
    //if VLC is stopped, return 1, otherwise 0
    if (strcmp("Stopped", pb_stat) == 0)
        return 1;   
    else
        return 0;
}

/* call the VLC Quit() method via DBus return 0 on success, 1 otherwise */
int quit_vlc(DBusConnection *conn, int pid) {
    DBusMessage *method_call;
    char name[80];

    //append the pid to the name of the destination
    sprintf (name, "%s%d", "org.mpris.MediaPlayer2.vlc-", pid);
    //initialize the Quit() method call message
    method_call = dbus_message_new_method_call(name,
        "/org/mpris/MediaPlayer2", "org.mpris.MediaPlayer2", "Quit");
    if (method_call == NULL) {
        log_error("Quit() method call NULL after creation");
        return 1;
    }
    //send the Quit() message, report error if any
    if (!dbus_connection_send(conn, method_call, NULL)) {
        log_error("Send of Quit() call message failed\n");
        return 1;
    }
    return 0;
}

/* return PID of VLC if it is running, 0 otherwise */
int vlc_running(DBusConnection *conn) {
    DBusError err;
    DBusMessage *method_call, *reply;
    DBusMessageIter iter, sub_iter;
    int arg_type = 0, pid = 0;
    const char *string;

    dbus_error_init(&err); // init DBus error
    method_call = dbus_message_new_method_call("org.freedesktop.DBus",
        "/org/freedesktop/DBus", "org.freedesktop.DBus", "ListNames");
    //call the method, get the reply, check for error
    reply = dbus_connection_send_with_reply_and_block (conn, 
    method_call, -1, &err);
    //initialize an iterator pointing to arguments in reply message
    if (!dbus_message_iter_init (reply, &iter)) {
        log_error("reply message has no arguments\n");
        return 1;
    }
    //check the type of the first argument (should be array)
    arg_type = dbus_message_iter_get_arg_type(&iter);
    if (arg_type != DBUS_TYPE_ARRAY) {
        log_error("Expected array, got %d\n", arg_type);
        return 1;
    }
    //recurse into the array container to get an iter to the string
    dbus_message_iter_recurse(&iter, &sub_iter);
    //check the type of the value in the array (should be string)
    arg_type = dbus_message_iter_get_arg_type(&sub_iter);
    if (arg_type != DBUS_TYPE_STRING) {
        log_error("Expected string, got %d\n", arg_type);
        return 1;
    }
    //extract the values of the strings from sub_iter
    while (dbus_message_iter_has_next(&sub_iter)) {
        dbus_message_iter_get_basic(&sub_iter, &string);
        if (strstr(string, "org.mpris.MediaPlayer2.vlc-") != NULL) {
            //extract the PID from the string
            sscanf(string, "org.mpris.MediaPlayer2.vlc-%d", &pid);
            return pid;
        }
        else //go to the next array element
           dbus_message_iter_next(&sub_iter);
    }
    //VLC interface was not found.  Assume VLC is not running.
    return 0;
}

/* Print the usage help then exit */
void usage(char *pname) {
    fprintf(stderr, "usage: %s start | stop\n", pname);
    exit(1);
}

/* Signal handler function */
void sig_handler(int signo) {
    if (signo == SIGUSR1) 
        running = 0;
    return;
}

/* Write errors to a log file */
void log_error(const char *format, ...)
{
    va_list args;
    FILE *errlog;
    time_t t = time(NULL);

    errlog = fopen("/tmp/vlcwderr.log", "a");

    fprintf(errlog, "%s ", asctime(localtime(&t)));
    va_start(args, format);
    vfprintf(errlog, format, args);
    va_end(args);

    fclose(errlog);
    return;
}
\$\endgroup\$
5
\$\begingroup\$

Define your variable in the smallest possible scope as it make things easier to understand.


Avoid hardcoded strings and magic numbers in your code. In you do need them, make sure you define them in a single place ( Don't Repeat Yourself ) .


Split your logic into smaller functions. In your case, one can easily see that start and stop are good candidates.


Re-opening and closing the file every time you need to log something does not seem like an efficient solution. In your case, it might do the trick.


You don't need to have return; at the end of a function.


You should try to use return to let the calling functions (try to) handle the errors instead of exit-ing from everywhere in the code.


Here's a non tested version of your code after taking into account these simple comments :

/* VLC-Watchdog v6
 *   by Jeremy B. Beck
 *
 * A fix for stopped VLC Media Player inhibiting the power management
 * daemon and preventing screen saver and/or monitor power off on my Ubuntu
 * 12.04 system using the DBus low level API.
 *
 * Takes start | stop as command line argument.
 *
 * Checks if VLC Media Player is running by searching for the interface in
 * the DBus list of interface names.  If VLC is found to be running,
 * queries the playback status through the DBus Get method.  If the
 * playback status is found to be stopped, calls the Quit() method via
 * DBus.  If VLC is found not to be running, VLC-Watchdog sleeps for 30
 * seconds then repeats all of the above.  When VLC-Watchdog receives
 * the stop signal (eg. "VLC-Watchdog stop" from the command line) it exits.
 *
 * Changed in v6:
 * + Check if VLC is running by looking for the interface name via DBus
 *   instead of using pgrep.
 * + Cleaned up code to connect to session bus.  Connect to bus in main()
 *   and pass the connection pointer to the methods that need it instead of
 *   creating a new connection in every method.
 *
 * Changed in v5:
 * + double check if VLC is stopped after 10 sec delay to prevent quitting
 *   VLC while it is starting up or loading media
 * Changed in v4:
 * + cleaned up error logging
 * + time stamped error log entries
 */

#include <stdio.h>
#include <stdlib.h>
#include <dbus/dbus.h>
#include <unistd.h>
#include <string.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <time.h>

static void skeleton_daemon();
int vlc_running(DBusConnection *conn);
int vlc_stopped(DBusConnection *conn, int pid);
int quit_vlc(DBusConnection *conn, int pid);
void usage(char *pname);
void sig_handler(int signo);
void log_error(const char *format, ...);

extern int running = 1;
extern int err_count = 0;

#define PATH_TO_VLCWATCHDOG_PID "/tmp/vlcwatchdog.pid"
#define PATH_TO_MEDIAPLAYER "/org/mpris/MediaPlayer2"

int main(int argc, char *argv[]) {
    if (argc==2)
    {
        //start / run actions
        if (strcmp(argv[1], "start") == 0) {
            return do_start();
        }
        //stop actions
        else if (strcmp(argv[1], "stop") == 0) {
            return do_stop();
        }
    }

    //check command line arguments are not valid 
    usage(argv[0]);
    exit(1);
}

int do_start()
{
    skeleton_daemon(); //daemonize
    //check if DBus is loaded
    if (getenv("DBUS_SESSION_BUS_ADDRESS") == NULL) {
        log_error("DBus session bus address == NULL\n");
        log_error("stopping VLC-Watchdog\n");
        return(1);
    }
    //write the PID to a file for future reference
    FILE * fp = fopen(PATH_TO_VLCWATCHDOG_PID, "w");
    fprintf(fp, "%d\n", getpid());
    fclose(fp);
    //register signal handler
    if (signal(SIGUSR1, sig_handler) == SIG_ERR) {
        log_error("error while setting signal handler\n");
        return(1);
    }
    /* when first started and every 30 seconds thereafter, check if
     * VLC is running, check if it is stopped, ask it to Quit() if it
     * is stopped.  Stop and exit when SIGUSR1 is received */
    DBusError err;
    dbus_error_init(&err); // init DBus error
    DBusConnection *conn = dbus_bus_get(DBUS_BUS_SESSION, &err); //connect to session bus
    if (conn == NULL) {
        log_error(err.message);
        return 1;
    }
    while (running) {
        if (int pid = vlc_running(conn)) {
            /* if VLC is stopped wait 10 sec then check if it is still
             * stopped in an attempt to prevent quitting VLC while it is
             * starting up or loading media */
            if (vlc_stopped(conn, pid)) {
                sleep(10);
                if (vlc_stopped(conn, pid)) {
                    if((quit_vlc(conn, pid)) == 1) {
                        log_error("quit_vlc() returned 1. exiting\n");
                        break;
                    }
                }
            }
        }
        if (err_count >= 10) {
            log_error("error count exceeded.  exiting\n");
            break;
        }
        sleep(30);
    }
    //remove temporary file
    if (remove(PATH_TO_VLCWATCHDOG_PID) == -1) {
        log_error("error removing temporary file %s",PATH_TO_VLCWATCHDOG_PID);
        return(1);
    }
    return 0;
}

int do_stop()
{
    //retrieve PID from file
    int pid;
    if (access(PATH_TO_VLCWATCHDOG_PID, R_OK) != -1) {
        FILE * fp = fopen(PATH_TO_VLCWATCHDOG_PID, "r");
        if (fscanf(fp, "%d", &pid) != 1) {
            log_error("fscanf failed to read PID from file while stopping\n");
        }
        fclose(fp);
    }
    //if unable to read PID from file, try pgrep
    else {
        char whatsmyname[256], s[32];
        sprintf(whatsmyname, "%s%s", "pgrep ", argv[0]);
        FILE * fp = popen(whatsmyname, "r");
        if (fp != NULL) {
            if (fgets(s, 32, fp) != NULL) {
                sscanf(s, "%d", &pid);
                fclose(fp);
            }
            else {
                fclose(fp);
                fprintf(stderr, "daemon does not appear to be running\n");
                return(1);
            }
        }
        else {
            fclose(fp);
            log_error("popen failed while stopping\n");
            return(1);
        }
    }
    //issue stop signal to VLC-Watchdog daemon
    kill(pid, SIGUSR1);
    return 0;
}



/* deamonize the process */
static void skeleton_daemon() {
    /* Fork off the parent process */
    pid_t pid = fork();

    /* An error occurred */
    if (pid < 0)
        exit(EXIT_FAILURE);

    /* Success: Let the parent terminate */
    if (pid > 0)
        exit(EXIT_SUCCESS);

    /* On success: The child process becomes session leader */
    if (setsid() < 0)
        exit(EXIT_FAILURE);

    /* Fork off for the second time*/
    pid = fork();

    /* An error occurred */
    if (pid < 0)
        exit(EXIT_FAILURE);

    /* Success: Let the parent terminate */
    if (pid > 0)
        exit(EXIT_SUCCESS);

    /* Set new file permissions */
    umask(0);

    /* Change the working directory to the root directory */
    /* or another appropriated directory */
    if (chdir("/") != 0) {
        log_error("chdir failed in skeleton_daemon\n");
        exit(1);
    }

    /* Close all open file descriptors */
    for (int x = sysconf(_SC_OPEN_MAX); x>0; x--)
    {
        close (x);
    }

}

/* check if VLC is stopped, if so, return 1, return 0 otherwise */
int vlc_stopped(DBusConnection *conn, int pid) {
    DBusError err;
    DBusMessageIter iter, sub_iter;
    const char *prop_iface = "org.mpris.MediaPlayer2.Player";
    const char *prop_iface_method_name = "PlaybackStatus";
    const char *pb_stat;
    char name[80];

    //append the pid to the name of the destination
    sprintf (name, "%s%d", "org.mpris.MediaPlayer2.vlc-", pid);
    dbus_error_init(&err); // init DBus error
    //create the method call to be used to get playback status of VLC
    DBusMessage * method_call = dbus_message_new_method_call(name,
            PATH_TO_MEDIAPLAYER, "org.freedesktop.DBus.Properties",
            "Get");
    if (method_call == NULL) {
        log_error("Get() method call is NULL after creation\n");
        return 1;
    }
    //put the method arguments on the message
    if (!dbus_message_append_args(method_call, DBUS_TYPE_STRING,
                &prop_iface, DBUS_TYPE_STRING, &prop_iface_method_name,
                DBUS_TYPE_INVALID)) {
        log_error("failed to append args to message\n");
        return 1;
    }
    //call the method, get the reply, check for error
    DBusMessage * reply = dbus_connection_send_with_reply_and_block (conn,
            method_call, -1, &err);
    //this error can be recovered from
    if (reply == NULL) {
        log_error("null reply \n %s \n", err.message);
        dbus_error_free(&err);
        err_count++;
        return 0;
    }
    //initialize an iterator pointing to arguments in reply message
    if (!dbus_message_iter_init (reply, &iter)) {
        log_error("reply message has no arguments\n");
        return 1;
    }
    //check the type of the first argument (should be variant)
    int arg_type = dbus_message_iter_get_arg_type(&iter);
    if (arg_type != DBUS_TYPE_VARIANT) {
        log_error("Expected variant, got %d\n", arg_type);
        return 1;
    }
    //recurse into the variant container to get an iter to the string
    dbus_message_iter_recurse(&iter, &sub_iter);
    //check the type of the value in the variant (should be string)
    arg_type = dbus_message_iter_get_arg_type(&sub_iter);
    if (arg_type != DBUS_TYPE_STRING) {
        log_error("Expected string, got %d\n", arg_type);
        return 1;
    }
    //extract the value of the string from sub_iter
    dbus_message_iter_get_basic(&sub_iter, &pb_stat);
    //if VLC is stopped, return 1, otherwise 0
    return (strcmp("Stopped", pb_stat) == 0)
}

/* call the VLC Quit() method via DBus return 0 on success, 1 otherwise */
int quit_vlc(DBusConnection *conn, int pid) {
    char name[80];

    //append the pid to the name of the destination
    sprintf (name, "%s%d", "org.mpris.MediaPlayer2.vlc-", pid);
    //initialize the Quit() method call message
    DBusMessage *method_call = dbus_message_new_method_call(name,
            PATH_TO_MEDIAPLAYER, "org.mpris.MediaPlayer2", "Quit");
    if (method_call == NULL) {
        log_error("Quit() method call NULL after creation");
        return 1;
    }
    //send the Quit() message, report error if any
    if (!dbus_connection_send(conn, method_call, NULL)) {
        log_error("Send of Quit() call message failed\n");
        return 1;
    }
    return 0;
}

/* return PID of VLC if it is running, 0 otherwise */
int vlc_running(DBusConnection *conn) {
    DBusError err;
    DBusMessageIter iter, sub_iter;

    dbus_error_init(&err); // init DBus error
    DBusMessage *method_call = dbus_message_new_method_call("org.freedesktop.DBus",
            "/org/freedesktop/DBus", "org.freedesktop.DBus", "ListNames");
    //call the method, get the reply, check for error
    DBusMessage * reply = dbus_connection_send_with_reply_and_block (conn,
            method_call, -1, &err);
    //initialize an iterator pointing to arguments in reply message
    if (!dbus_message_iter_init (reply, &iter)) {
        log_error("reply message has no arguments\n");
        return 1;
    }
    //check the type of the first argument (should be array)
    int arg_type = dbus_message_iter_get_arg_type(&iter);
    if (arg_type != DBUS_TYPE_ARRAY) {
        log_error("Expected array, got %d\n", arg_type);
        return 1;
    }
    //recurse into the array container to get an iter to the string
    dbus_message_iter_recurse(&iter, &sub_iter);
    //check the type of the value in the array (should be string)
    arg_type = dbus_message_iter_get_arg_type(&sub_iter);
    if (arg_type != DBUS_TYPE_STRING) {
        log_error("Expected string, got %d\n", arg_type);
        return 1;
    }
    //extract the values of the strings from sub_iter
    while (dbus_message_iter_has_next(&sub_iter)) {
        const char *string;
        dbus_message_iter_get_basic(&sub_iter, &string);
        if (strstr(string, "org.mpris.MediaPlayer2.vlc-") != NULL) {
            //extract the PID from the string
            int pid;
            sscanf(string, "org.mpris.MediaPlayer2.vlc-%d", &pid);
            return pid;
        }
        else //go to the next array element
            dbus_message_iter_next(&sub_iter);
    }
    //VLC interface was not found.  Assume VLC is not running.
    return 0;
}

/* Print the usage help then exit */
void usage(char *pname) {
    fprintf(stderr, "usage: %s start | stop\n", pname);
}

/* Signal handler function */
void sig_handler(int signo) {
    if (signo == SIGUSR1)
        running = 0;
}

/* Write errors to a log file */
void log_error(const char *format, ...)
{
    va_list args;
    time_t t = time(NULL);

    FILE *errlog = fopen("/tmp/vlcwderr.log", "a");
    fprintf(errlog, "%s ", asctime(localtime(&t)));
    va_start(args, format);
    vfprintf(errlog, format, args);
    va_end(args);

    fclose(errlog);
    return;
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ You should not use sprint because it's unsafe and easy to fall in buffer overflows, especially when user's activity is included. Rather go for snprintf. \$\endgroup\$
    – edmz
    Apr 1 '14 at 17:53
  • \$\begingroup\$ This is probably worth an answer on its own. \$\endgroup\$
    – SylvainD
    Apr 1 '14 at 17:54
  • 1
    \$\begingroup\$ There are some other things I want to point out, I'll post an answer later. \$\endgroup\$
    – edmz
    Apr 1 '14 at 17:57
4
\$\begingroup\$

Other things that should be considered are:

  • In main() exit(1); why not just return 1?
  • When dealing with command line options, you should generally make use of getopt and getopt_long, although in this case is not that useful.
  • In the C standard library, there are a lot of unsafe functions and you should avoid them, of course. You can find some here.
    In your case, avoid using sprintf and sscanf*, especially when user's input is involved. That easily leads to buffer overflows.
  • fopen might fail and you want to know that. If you don't want, you'll likely get a nice SIGSEGV.
  • Sometimes you use macros for string literals, sometimes const char*. I'd personally prefer just one choise, the last one precisely.

*there is no snscanf, because *scanf lets you specify buffer's size in the format string. For example:

printf("Enter a string: ");
char s[128];
scanf("%128s", s);

but for strings, use fgets.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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