[patches] [say 1/2] say: Wrap logrotate signal handler with ev_signal

[Georgy Kirichenko]

This seems as a good patch but there is one more issue:
coio_task_post does an yield, but we don't want it on tasks posting, because 
logs list may be changed while we are in a curent task waiting.
I think the better way is to modify coio_task_post to not to do any yield if 
timeout is zero and use it while log rotate tasks posting.
@Kostya, please get a feedback

On Thursday, February 15, 2018 6:10:01 PM MSK imarkov wrote:
> From: IlyaMarkovMipt <markovilya197 at gmail.com>
> 
> Current log rotation is not async signal safe.
> In order to make it so refactor signal handling
> with ev_signal.
> 
> Log rotation for each logger performs in separate
> coio_task to provide async and thread-safe execution.
> 
> Relates #3015
> ---
>  src/lua/log.lua |  8 ++++++--
>  src/main.cc     |  3 ++-
>  src/say.c       | 62
> +++++++++++++++++++++++++++++++++++++++++++++++---------- src/say.h       |
>  5 ++++-
>  4 files changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/src/lua/log.lua b/src/lua/log.lua
> index b90eb88..0ac0e8f 100644
> --- a/src/lua/log.lua
> +++ b/src/lua/log.lua
> @@ -12,7 +12,11 @@ ffi.cdef[[
> 
> 
>      extern sayfunc_t _say;
> -    extern void say_logrotate(int);
> +    extern struct ev_loop;
> +    extern struct ev_signal;
> +
> +    extern void
> +    say_logrotate(struct ev_loop *, struct ev_signal *, int);
> 
>      enum say_level {
>          S_FATAL,
> @@ -104,7 +108,7 @@ local function say_closure(lvl)
>  end
> 
>  local function log_rotate()
> -    ffi.C.say_logrotate(0)
> +    ffi.C.say_logrotate(nil, nil, 0)
>  end
> 
>  local function log_level(level)
> diff --git a/src/main.cc b/src/main.cc
> index ec06103..1682bae 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -83,7 +83,7 @@ static char *pid_file = NULL;
>  static char **main_argv;
>  static int main_argc;
>  /** Signals handled after start as part of the event loop. */
> -static ev_signal ev_sigs[5];
> +static ev_signal ev_sigs[6];
>  static const int ev_sig_count = sizeof(ev_sigs)/sizeof(*ev_sigs);
> 
>  static double start_time;
> @@ -335,6 +335,7 @@ signal_init(void)
>  	ev_signal_init(&ev_sigs[2], signal_cb, SIGTERM);
>  	ev_signal_init(&ev_sigs[3], signal_cb, SIGHUP);
>  	ev_signal_init(&ev_sigs[4], signal_sigwinch_cb, SIGWINCH);
> +	ev_signal_init(&ev_sigs[5], say_logrotate, SIGHUP);
>  	for (int i = 0; i < ev_sig_count; i++)
>  		ev_signal_start(loop(), &ev_sigs[i]);
> 
> diff --git a/src/say.c b/src/say.c
> index 65808a6..9d2a174 100644
> --- a/src/say.c
> +++ b/src/say.c
> @@ -42,6 +42,7 @@
>  #include <sys/socket.h>
>  #include <sys/un.h>
>  #include <arpa/inet.h>
> +#include <coio_task.h>
> 
>  pid_t log_pid = 0;
>  int log_level = S_INFO;
> @@ -265,26 +266,66 @@ log_rotate(const struct log *log)
>  	ssize_t r = write(log->fd,
>  			  logrotate_message, (sizeof logrotate_message) - 1);
>  	(void) r;
> +	/*
> +	 * log_background applies only to log_default logger
> +	 */
> +	if (log == log_default && log_background &&
> +		log->type == SAY_LOGGER_FILE) {
> +		dup2(log_default->fd, STDOUT_FILENO);
> +		dup2(log_default->fd, STDERR_FILENO);
> +	}
> +
> +	return 0;
> +}
> +
> +struct rotate_task {
> +	struct coio_task base;
> +	struct log *log;
> +};
> +
> +static int
> +logrotate_cb(struct coio_task *ptr)
> +{
> +	struct rotate_task *task = (struct rotate_task *) ptr;
> +	if (log_rotate(task->log) < 0) {
> +		diag_log();
> +	}
> +	return 0;
> +}
> +
> +static int
> +logrotate_cleanup_cb(struct coio_task *ptr)
> +{
> +	struct rotate_task *task = (struct rotate_task *) ptr;
> +	coio_task_destroy(&task->base);
> +	free(task);
>  	return 0;
>  }
> 
>  void
> -say_logrotate(int signo)
> +say_logrotate(struct ev_loop *loop, struct ev_signal *w, int revents)
>  {
> -	(void) signo;
> +	(void) loop;
> +	(void) w;
> +	(void) revents;
>  	int saved_errno = errno;
>  	struct log *log;
>  	rlist_foreach_entry(log, &log_rotate_list, in_log_list) {
> -		if (log_rotate(log) < 0) {
> +		struct rotate_task *task =
> +			(struct rotate_task *) calloc(1, sizeof(*task));
> +		if (task == NULL) {
> +			diag_set(OutOfMemory, sizeof(*task), "malloc",
> +				 "say_logrotate");
>  			diag_log();
> +			continue;
>  		}
> -	}
> -	/*
> -	 * log_background applies only to log_default logger
> -	 */
> -	if (log_background && log_default->type == SAY_LOGGER_FILE) {
> -		dup2(log_default->fd, STDOUT_FILENO);
> -		dup2(log_default->fd, STDERR_FILENO);
> +		coio_task_create(&task->base, logrotate_cb, logrotate_cleanup_cb);
> +		task->log = log;
> +		if (coio_task_post(&task->base, TIMEOUT_INFINITY) != 0) {
> +			say_error("log_rotate: failed to post a task");
> +			continue;
> +		}
> +		logrotate_cleanup_cb(&task->base);
>  	}
>  	errno = saved_errno;
>  }
> @@ -535,7 +576,6 @@ say_logger_init(const char *init_str, int level, int
> nonblock, say_set_log_level(level);
>  	log_background = background;
>  	log_pid = log_default->pid;
> -	signal(SIGHUP, say_logrotate);
>  	say_set_log_format(say_format_by_name(format));
> 
>  	if (background) {
> diff --git a/src/say.h b/src/say.h
> index 1b63229..46e6976 100644
> --- a/src/say.h
> +++ b/src/say.h
> @@ -185,8 +185,11 @@ say_set_log_format(enum say_format format);
>  enum say_format
>  say_format_by_name(const char *format);
> 
> +struct ev_loop;
> +struct ev_signal;
> +
>  void
> -say_logrotate(int /* signo */);
> +say_logrotate(struct ev_loop *, struct ev_signal *, int /* revents */);
> 
>  /** Init default logger. */
>  void

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180216/092f8b86/attachment.sig>