Hello,
attached are the first two patches, one fixing the actual bug I
encountered and one just tidying up the signal handling a little. More
to come.
Are they ok like this?
Cheers,
Conrad
On 07/25/2014 11:04 AM, Conrad Hoffmann wrote:
> Hey,
>
> On 07/25/2014 08:31 AM, Willy Tarreau wrote:
>>> 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).
>>
>> From what I remember, the purpose was to be able to upgrade the wrapper
>> itself without having to kill it. Typically in order to apply changes
>> like you just performed... So I think you should bring that feature back.
>
> Oh, right, I actually knew that before. Another proof of the importance
> of sometimes taking a step back. Also, I'll add a comment :)
>
>> You cleanup patch is interesting but it does too many things at once for
>> a single patch, I'd strongly prefer if you would cut it into pieces each
>> addressing a specific issue. It would make the code more easily reviewable
>> and would also help troubleshooting in the event it would cause any minor
>> regression.
>
> Sounds very reasonable. I'll see if I can break it in chunks that would
> be independently revertable, although I think that may be difficult, but
> at least things will be easier to review and merge. Will get back soon!
>
> 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 34ac06bae5856d28773a194e7d2b1ee508213c39 Mon Sep 17 00:00:00 2001
From: Conrad Hoffmann <con...@soundcloud.com>
Date: Mon, 28 Jul 2014 23:22:43 +0200
Subject: [PATCH 1/2] Fix search for -p argument in systemd wrapper.
Searching for the pid file in the list of arguments did not
take flags without parameters into account, like e.g. -de. Because
of this, the wrapper would use a different pid file than haproxy
if such an argument was specified before -p.
The new version can still yield a false positive for some crazy
situations, like your config file name starting with "-p", but
I think this is as good as it gets without using getopt or some
library.
Signed-off-by: Conrad Hoffmann <con...@soundcloud.com>
---
src/haproxy-systemd-wrapper.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c
index ba07ebe..529b213 100644
--- a/src/haproxy-systemd-wrapper.c
+++ b/src/haproxy-systemd-wrapper.c
@@ -130,11 +130,8 @@ static void sigint_handler(int signum __attribute__((unused)))
static void init(int argc, char **argv)
{
while (argc > 1) {
- if (**argv == '-') {
- char *flag = *argv + 1;
- --argc; ++argv;
- if (*flag == 'p')
- pid_file = *argv;
+ if ((*argv)[0] == '-' && (*argv)[1] == 'p') {
+ pid_file = *(argv + 1);
}
--argc; ++argv;
}
--
2.0.2
>From da798ecab739c6287290400c42d876f305a56174 Mon Sep 17 00:00:00 2001
From: Conrad Hoffmann <con...@soundcloud.com>
Date: Mon, 28 Jul 2014 23:52:20 +0200
Subject: [PATCH 2/2] Improve signal handling in systmd wrapper.
Move all code out of the signal handlers, since this is potentially
dangerous. To make sure the signal handlers behave as expected, use
sigaction() instead of signal(). That also obsoletes messing with
the signal mask after restart.
Signed-off-by: Conrad Hoffmann <con...@soundcloud.com>
---
src/haproxy-systemd-wrapper.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/haproxy-systemd-wrapper.c b/src/haproxy-systemd-wrapper.c
index 529b213..90a94ce 100644
--- a/src/haproxy-systemd-wrapper.c
+++ b/src/haproxy-systemd-wrapper.c
@@ -22,6 +22,8 @@
#define SD_DEBUG "<7>"
#define SD_NOTICE "<5>"
+static volatile sig_atomic_t caught_signal;
+
static char *pid_file = "/run/haproxy.pid";
static int wrapper_argc;
static char **wrapper_argv;
@@ -103,7 +105,12 @@ static int read_pids(char ***pid_strv)
return read;
}
-static void sigusr2_handler(int signum __attribute__((unused)))
+static void signal_handler(int signum)
+{
+ caught_signal = signum;
+}
+
+static void do_restart(void)
{
setenv(REEXEC_FLAG, "1", 1);
fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: re-executing\n");
@@ -111,7 +118,7 @@ static void sigusr2_handler(int signum __attribute__((unused)))
execv(wrapper_argv[0], wrapper_argv);
}
-static void sigint_handler(int signum __attribute__((unused)))
+static void do_shutdown(void)
{
int i, pid;
char **pid_strv = NULL;
@@ -147,25 +154,21 @@ int main(int argc, char **argv)
--argc; ++argv;
init(argc, argv);
- signal(SIGINT, &sigint_handler);
- signal(SIGUSR2, &sigusr2_handler);
+ struct sigaction sa;
+ memset(&sa, 0, sizeof(struct sigaction));
+ sa.sa_handler = &signal_handler;
+ sigaction(SIGUSR2, &sa, NULL);
+ sigaction(SIGINT, &sa, NULL);
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;
unsetenv(REEXEC_FLAG);
spawn_haproxy(pid_strv, nb_pid);
- /* 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);
-
for (i = 0; i < nb_pid; ++i)
free(pid_strv[i]);
free(pid_strv);
@@ -176,8 +179,16 @@ int main(int argc, char **argv)
}
status = -1;
- while (-1 != wait(&status) || errno == EINTR)
- ;
+ while (-1 != wait(&status) || errno == EINTR) {
+ if (caught_signal == SIGUSR2) {
+ caught_signal = 0;
+ do_restart();
+ }
+ else if (caught_signal == SIGINT) {
+ caught_signal = 0;
+ do_shutdown();
+ }
+ }
fprintf(stderr, SD_NOTICE "haproxy-systemd-wrapper: exit, haproxy RC=%d\n",
status);
--
2.0.2
