From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 8 Aug 2018 13:28:37 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH] Fix O_NONBLOCK flag loss
Message-ID: <20180808102837.fsv45ojkrtqsj2zu@esperanza>
References: <20180807205742.52303-1-krishtal.olja@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180807205742.52303-1-krishtal.olja@gmail.com>
To: Olga Arkhangelskaia <krishtal.olja@gmail.com>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Tue, Aug 07, 2018 at 11:57:42PM +0300, Olga Arkhangelskaia wrote:
> During syslog reconnect we lose nonblock flag. This leads to misbehavior
> while logging. Tarantool hangs forever
> 
> Closes #3615
> ---
> https://github.com/tarantool/tarantool/issues/3615
> https://github.com/tarantool/tarantool/tree/OKriw/gh-3615-loss-nonblock-1.10

The milestone has changed to 1.9. Please rebase your patch.

> 
>  src/say.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)

First of all, please write a test case that demonstrates that tarantool
hangs without this patch. To do that, you need to use a custom syslog
destination (once the corresponding patch is merged). Since this patch
is going to be based on 1.9 while configurable syslog destination will
only be available in 1.10, you'll have to submit a fix and a test in
separate patches - for 1.9 and 1.10, respectively.

> 
> diff --git a/src/say.c b/src/say.c
> index 063e0295c..4bd47b1f2 100644
> --- a/src/say.c
> +++ b/src/say.c
> @@ -238,6 +238,16 @@ write_to_file(struct log *log, int total);
>  static void
>  write_to_syslog(struct log *log, int total);
>  
> +static void
> +set_nonblock(int fd)
> +{
> +	int flags;
> +	if ( (flags = fcntl(fd, F_GETFL, 0)) < 0 ||
> +	     fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0) {
> +		say_syserror("fcntl, fd=%i", fd);
> +	}
> +}
> +

I guess, we can make this function take a 'log' object and do nothing
if log->nonblock is unset (we check that flag before each call anyway).
I mean

	log_set_nonblock(struct log *log)
	{
		if (!log->nonblock)
			return;
		...
	}

IMO this would look better. Also, please write a short comment
explaining what this function does - according to our coding style,
every function should have a comment.

>  /**
>   * Rotate logs on SIGHUP
>   */
> @@ -262,13 +272,9 @@ log_rotate(struct log *log)
>  	dup2(fd, log->fd);
>  	close(fd);
>  
> -	if (log->nonblock) {
> -		int flags;
> -		if ( (flags = fcntl(log->fd, F_GETFL, 0)) < 0 ||
> -		     fcntl(log->fd, F_SETFL, flags | O_NONBLOCK) < 0) {
> -			say_syserror("fcntl, fd=%i", log->fd);
> -		}
> -	}
> +	if (log->nonblock)
> +		set_nonblock(log->fd);
> +

There's one more place where we set O_NONBLOCK - log_create. Please use
the new function there as well.

BTW do we really need to set O_NONBLOCK in log_rotate? AFAIU it is only
called for file loggers which are incompatible with nonblocking mode.
May be, an assertion would do? However, we don't check log parameters in
1.9 (only in 2.0) so I guess we should leave this code ...

>  	/* We are in ev signal handler
>  	 * so we don't have to be worry about async signal safety
>  	 */
> @@ -940,6 +946,8 @@ write_to_syslog(struct log *log, int total)
>  			close(log->fd);
>  		log->fd = log_syslog_connect(log);
>  		if (log->fd >= 0) {
> +			if (log->nonblock)
> +				set_nonblock(log->fd);
>  			/*
>  			 * In a case or error the log message is
>  			 * lost. We can not wait for connection -