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 -
prev parent 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