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

[Konstantin Osipov]

* Georgy Kirichenko <georgy at tarantool.org> [18/02/16 12:08]:
> 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

I'm OK with being able to fire and forget a coio task, but the way
the new function is used in the patch frees the task object before 
it's used in the thread pool worker thread (please see below).

Given this problem has been lingering for so long time, I would
kindly ask to come up with a unit test for log rotation.

Thanks.

> >  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;
> > +}

What happens if the log is destroyed while it's being rotated?
Seems like we also need a create/destroy/rotate mutex in each
log object.

Please cover log destruction during rotation in a unit test. 

> > +
> > +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);

Here we free the task before it's been looked at by a thread pool
thread. Had we had a unit test, this would not have passed
unnoticed.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov