Hey Willy, I know you're not that much into the systemd stuff, but although we don't use systemd we actually us the wrapper a lot, and I thought it could use a little make-over.
There was only one small bug really, but once I got going I thought i might as well simplify it a litle. I pulled everything out of the signal handlers, switched from signal() to sigaction(), got rid of global state and got rid of the wrapper exec()'ing itself (not sure if there was some reason for that, but it seems to be unneccessary). It is late at night and I might have to revise a few things after some sleep, but I wanted to get this out for you and others to look at. It is against tonights master of the haproxy-1.5 repo, I could also create it for 1.6 instead (or maybe it even applies out of the box). Keep it up and merci beaucoup, Conrad -- Conrad Hoffmann Traffic Engineer SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany Managing Director: Alexander Ljung | Incorporated in England & Wales with Company No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B
>From 582490ce909040285153a9644530c8913927a462 Mon Sep 17 00:00:00 2001 From: Conrad Hoffmann <[email protected]> Date: Fri, 25 Jul 2014 02:58:51 +0200 Subject: [PATCH] Improve and simplify systemd-wrapper. The signal handling did too much work in the context of the signal handler, which didn't yet cause problems (for me) but is to be avoided. While at it I switched from the deprecated signal() to the slightly more modern sigaction(). I also got rid of the wrapper re-executing itself on SIGUSR2, unless I overlooked something this was not needed. Moving stuff out of the signal handlers also obsoleted all but one global variables, making the code much more readable. Finally this also fixes an actual bug, where the -p switch that the wrapper looks for would not be found if an odd number of argument-less switches is used before it, e.g.: haproxy-systemd-wrapper -de -p mypidfile.pid would not work this way, but only if -de is placed at the end. Signed-off-by: Conrad Hoffmann <[email protected]> --- src/haproxy-systemd-wrapper.c | 272 +++++++++++++++++++++++------------------- 1 file changed, 151 insertions(+), 121 deletions(-) diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c index ba07ebe..01dd74c 100644 --- a/src/haproxy-systemd-wrapper.c +++ b/src/haproxy-systemd-wrapper.c @@ -18,171 +18,201 @@ #include <unistd.h> #include <sys/wait.h> -#define REEXEC_FLAG "HAPROXY_SYSTEMD_REEXEC" -#define SD_DEBUG "<7>" -#define SD_NOTICE "<5>" +/* Taken from sd-daemon.h to not actually depend on systemd. */ +#define SD_CRIT "<2>" /* critical conditions */ +#define SD_NOTICE "<5>" /* normal but significant condition */ +#define SD_DEBUG "<7>" /* debug-level messages */ -static char *pid_file = "/run/haproxy.pid"; -static int wrapper_argc; -static char **wrapper_argv; +#define WRAPPER_CRIT SD_CRIT "haproxy-systemd-wrapper: " +#define WRAPPER_NOTICE SD_NOTICE "haproxy-systemd-wrapper: " +#define WRAPPER_DEBUG SD_DEBUG "haproxy-systemd-wrapper: " -static void locate_haproxy(char *buffer, size_t buffer_size) -{ - char *end = NULL; +#define PATH_BUF_SIZE 512 +#define DEFAULT_PID_FILE "/run/haproxy.pid" + + +static volatile sig_atomic_t sig_caught = 0; - if (readlink("/proc/self/exe", buffer, buffer_size) > 0) - end = strrchr(buffer, '/'); +static void signal_handler(int signum) +{ + sig_caught = signum; +} - if (end == NULL) { - strncpy(buffer, "/usr/sbin/haproxy", buffer_size); - return; +/* Find the absolute path to haproxy executable, result must be free'd. */ +static char *locate_haproxy(void) +{ + char buffer[PATH_BUF_SIZE]; + + /* Assume haproxy executable is in same directory as the wrapper. */ + if (readlink("/proc/self/exe", buffer, PATH_BUF_SIZE) > 0) { + char *end = strrchr(buffer, '/'); + if (end) { + strncpy(end + 1, "haproxy", buffer + PATH_BUF_SIZE - (end + 1)); + buffer[PATH_BUF_SIZE - 1] = '\0'; + return strdup(buffer); + } } - end[1] = '\0'; - strncpy(end + 1, "haproxy", buffer + buffer_size - (end + 1)); - buffer[buffer_size - 1] = '\0'; + /* Emergency solution: assume some common default. */ + return strdup("/usr/sbin/haproxy"); } -static void spawn_haproxy(char **pid_strv, int nb_pid) +/* Spawn a new instance of haproxy, appending PIDs to command line as required. */ +static void spawn_haproxy(int argc, char **argv, char **pids) { - char haproxy_bin[512]; - pid_t pid; - int main_argc; - char **main_argv; - - main_argc = wrapper_argc - 1; - main_argv = wrapper_argv + 1; - - pid = fork(); - if (!pid) { - /* 3 for "haproxy -Ds -sf" */ - char **argv = calloc(4 + main_argc + nb_pid + 1, sizeof(char *)); - int i; - int argno = 0; - locate_haproxy(haproxy_bin, 512); - argv[argno++] = haproxy_bin; - for (i = 0; i < main_argc; ++i) - argv[argno++] = main_argv[i]; - argv[argno++] = "-Ds"; - if (nb_pid > 0) { - argv[argno++] = "-sf"; - for (i = 0; i < nb_pid; ++i) - argv[argno++] = pid_strv[i]; + int total_argc = argc + 1; /* NULL termination. */ + char **p = pids; + if (p) { + total_argc++; /* -sf */ + while (*p) { + total_argc++; /* PIDs */ + p++; } - argv[argno] = NULL; + } - fprintf(stderr, SD_DEBUG "haproxy-systemd-wrapper: executing "); - for (i = 0; argv[i]; ++i) - fprintf(stderr, "%s ", argv[i]); - fprintf(stderr, "\n"); + int i; + char **total_argv = calloc(total_argc, sizeof(char *)); + for (i = 0; i < argc; i++) { + total_argv[i] = argv[i]; + } + if (pids) { + total_argv[i++] = "-sf"; + p = pids; + while (*p) { + total_argv[i++] = *(p++); + } + } + total_argv[i] = NULL; - execv(argv[0], argv); - exit(0); + fprintf(stderr, WRAPPER_DEBUG "executing "); + for (i = 0; total_argv[i]; ++i) { + fprintf(stderr, "%s ", total_argv[i]); + } + fprintf(stderr, "\n"); + + int pid = fork(); + if (!pid) { /* Child process. */ + execv(total_argv[0], total_argv); + perror(WRAPPER_CRIT "execv failed"); + exit(EXIT_FAILURE); + } + else if (pid == -1) { + perror(WRAPPER_CRIT "fork failed"); } } -static int read_pids(char ***pid_strv) +/* Returns a NULL-terminated array of PID strings. */ +static char **read_pids(const char *pid_file) { + char **pids = calloc(1, sizeof(char*)); + FILE *f = fopen(pid_file, "r"); - int read = 0, allocated = 8; - char pid_str[10]; + if (!f) { + return pids; + } - if (!f) - return 0; + char pid_buf[16]; + int count = 0; - *pid_strv = malloc(allocated * sizeof(char *)); - while (1 == fscanf(f, "%s\n", pid_str)) { - if (read == allocated) { - allocated *= 2; - *pid_strv = realloc(*pid_strv, allocated * sizeof(char *)); - } - (*pid_strv)[read++] = strdup(pid_str); + while (1 == fscanf(f, "%s\n", pid_buf)) { + pids = realloc(pids, (++count + 1) * sizeof(char *)); + pids[count - 1] = strdup(pid_buf); } - fclose(f); - return read; + pids[count] = NULL; + return pids; } -static void sigusr2_handler(int signum __attribute__((unused))) +/* Free a list of pids allocated by read_pids(). */ +static inline void free_pids(char **pids) { - setenv(REEXEC_FLAG, "1", 1); - fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: re-executing\n"); + char **p = pids; + while (p && *p) { + free(*(p++)); + } + free(pids); +} - execv(wrapper_argv[0], wrapper_argv); +/* Determine PID file location, either from -p switch or a default location. */ +static const char *locate_pid_file(int argc, char **argv) +{ + while (argc > 1) { + if ((*argv)[0] == '-' && (*argv)[1] == 'p') { + return *(argv + 1); + } + --argc; ++argv; + } + return DEFAULT_PID_FILE; } -static void sigint_handler(int signum __attribute__((unused))) +/* Send SIGINT to all haproxy worker threads. */ +static void do_shutdown(const char *pid_file) { - int i, pid; - char **pid_strv = NULL; - int nb_pid = read_pids(&pid_strv); - for (i = 0; i < nb_pid; ++i) { - pid = atoi(pid_strv[i]); + char **pids = read_pids(pid_file); + char **p = pids; + while (p && *p) { + int pid = atoi(*(p++)); if (pid > 0) { fprintf(stderr, SD_DEBUG "haproxy-systemd-wrapper: SIGINT -> %d\n", pid); kill(pid, SIGINT); - free(pid_strv[i]); } } - free(pid_strv); + free_pids(pids); } -static void init(int argc, char **argv) +/* Spawn new haproxy instance that knows about the old one, taking over gracefully. */ +static void do_restart(int argc, char **argv, const char *pid_file) { - while (argc > 1) { - if (**argv == '-') { - char *flag = *argv + 1; - --argc; ++argv; - if (*flag == 'p') - pid_file = *argv; - } - --argc; ++argv; - } + fprintf(stderr, WRAPPER_NOTICE "re-executing\n"); + char **pids = read_pids(pid_file); + spawn_haproxy(argc, argv, pids); + free_pids(pids); } -int main(int argc, char **argv) +/* Install signal handlers. */ +static void setup_signals(void) { - int status; - - wrapper_argc = argc; - wrapper_argv = argv; - - --argc; ++argv; - init(argc, argv); + struct sigaction sa; + memset(&sa, 0, sizeof(struct sigaction)); + sa.sa_handler = &signal_handler; - signal(SIGINT, &sigint_handler); - signal(SIGUSR2, &sigusr2_handler); - - if (getenv(REEXEC_FLAG) != NULL) { - /* We are being re-executed: restart HAProxy gracefully */ - int i; - char **pid_strv = NULL; - int nb_pid = read_pids(&pid_strv); - sigset_t sigs; + if (sigaction(SIGUSR2, &sa, NULL) == -1 || sigaction(SIGINT, &sa, NULL) == -1) { + perror(WRAPPER_CRIT "sigaction failed"); + exit(EXIT_FAILURE); + } +} - unsetenv(REEXEC_FLAG); - spawn_haproxy(pid_strv, nb_pid); +int main(int argc, char **argv) +{ + const char *pid_file = locate_pid_file(argc, argv); + char *haproxy_binary = locate_haproxy(); + setup_signals(); + + int haproxy_argc = argc + 1; /* Adding -Ds */ + char **haproxy_argv = calloc(haproxy_argc, sizeof(char *)); + int i; + haproxy_argv[0] = haproxy_binary; + for (i = 1; i < argc; i++) { + haproxy_argv[i] = argv[i]; + } + haproxy_argv[i] = "-Ds"; - /* Unblock SIGUSR2 which was blocked by the signal handler - * before re-exec */ - sigprocmask(SIG_BLOCK, NULL, &sigs); - sigdelset(&sigs, SIGUSR2); - sigprocmask(SIG_SETMASK, &sigs, NULL); + spawn_haproxy(haproxy_argc, haproxy_argv, NULL); - for (i = 0; i < nb_pid; ++i) - free(pid_strv[i]); - free(pid_strv); - } - else { - /* Start a fresh copy of HAProxy */ - spawn_haproxy(NULL, 0); + int status = -1; + while (-1 != wait(&status) || errno == EINTR) { + if (sig_caught == SIGINT) { + sig_caught = 0; + do_shutdown(pid_file); + } + else if (sig_caught == SIGUSR2) { + sig_caught = 0; + do_restart(haproxy_argc, haproxy_argv, pid_file); + } } - status = -1; - while (-1 != wait(&status) || errno == EINTR) - ; - - fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: exit, haproxy RC=%d\n", - status); + fprintf(stderr, WRAPPER_NOTICE "exiting (%d)\n", WEXITSTATUS(status)); + free(haproxy_binary); + free(haproxy_argv); return status; } -- 2.0.2
