Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Olga Arkhangelskaia <krishtal.olja@gmail.com>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] [PATCH] Fix O_NONBLOCK flag loss
Date: Wed, 8 Aug 2018 13:28:37 +0300	[thread overview]
Message-ID: <20180808102837.fsv45ojkrtqsj2zu@esperanza> (raw)
In-Reply-To: <20180807205742.52303-1-krishtal.olja@gmail.com>

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 -

      reply	other threads:[~2018-08-08 10:28 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 20:57 Olga Arkhangelskaia
2018-08-08 10:28 ` Vladimir Davydov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180808102837.fsv45ojkrtqsj2zu@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=krishtal.olja@gmail.com \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH] Fix O_NONBLOCK flag loss' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox