From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 26 Mar 2019 20:26:02 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH] log: add missing assert into log_set_format() Message-ID: <20190326172602.ahn4h4v22t65yuzc@esperanza> References: <20190302172946.94121-1-roman.habibov@tarantool.org> <20190305142140.3mguirkwwsk3waho@esperanza> <43D75107-60D1-4598-B342-634AC7296DA7@tarantool.org> <20190319083008.f3qma6b4vbhfolyk@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Roman Khabibov Cc: tarantool-patches@freelists.org List-ID: On Mon, Mar 25, 2019 at 04:03:54PM +0300, Roman Khabibov wrote: > commit 4bc90124d0cb27f8eec4a0e1d656fcbb35b919dc > Author: Roman Khabibov > Date: Sat Mar 2 20:12:11 2019 +0300 > > say: throw error when "json" used with syslog > > Add function returning logger type to throw error from lua > when "json" used with syslog. > > Closes #3946 The commit message isn't very descriptive. Changed it to say: fix assertion in log_format when called for boot or syslog logger It's OK to use json format with the boot logger. As for syslog, let's add a check to Lua's log_format() so that it fails gracefully rather than raising an assertion. Closes #3946 > > diff --git a/extra/exports b/extra/exports > index d72421123..21fa9bf00 100644 > --- a/extra/exports > +++ b/extra/exports > @@ -65,6 +65,7 @@ mp_encode_float > mp_decode_double > mp_decode_float > > +log_type > say_set_log_level > say_logrotate > say_set_log_format > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index 50ee55692..aa3eb37d3 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -161,6 +161,12 @@ level_to_syslog_priority(int level) > } > } > > +enum say_logger_type > +log_type() > +{ > + return log_default->type; > +} > + > void > log_set_level(struct log *log, enum say_level level) > { > @@ -170,7 +176,8 @@ log_set_level(struct log *log, enum say_level level) > void > log_set_format(struct log *log, log_format_func_t format_func) > { > - assert(format_func == say_format_plain || > + assert(format_func == say_format_json || > + format_func == say_format_plain || > log->type == SAY_LOGGER_STDERR || > log->type == SAY_LOGGER_PIPE || log->type == SAY_LOGGER_FILE); This is a useless assertion now. Changed it to assert(format_func == say_format_plain || log->type != SAY_LOGGER_SYSLOG); > > diff --git a/src/lib/core/say.h b/src/lib/core/say.h > index 70050362c..d26c3ddef 100644 > --- a/src/lib/core/say.h > +++ b/src/lib/core/say.h > @@ -195,6 +195,13 @@ int > log_say(struct log *log, int level, const char *filename, > int line, const char *error, const char *format, ...); > > +/** > + * Default logger type info. > + * @retval say_logger_type. > + */ > +enum say_logger_type > +log_type(); > + > /** > * Set log level. Can be used dynamically. > * > diff --git a/src/lua/log.lua b/src/lua/log.lua > index 0ac0e8f26..fe8f8921f 100644 > --- a/src/lua/log.lua > +++ b/src/lua/log.lua > @@ -4,10 +4,22 @@ local ffi = require('ffi') > ffi.cdef[[ > typedef void (*sayfunc_t)(int level, const char *filename, int line, > const char *error, const char *format, ...); > + > + enum say_logger_type { > + SAY_LOGGER_BOOT, > + SAY_LOGGER_STDERR, > + SAY_LOGGER_FILE, > + SAY_LOGGER_PIPE, > + SAY_LOGGER_SYSLOG > + }; > + > + enum say_logger_type > + log_type(); > + > void > say_set_log_level(int new_level); > > - void > + int Stray change. Removed it. > say_set_log_format(enum say_format format); > > > @@ -117,6 +129,9 @@ end > > local function log_format(format_name) > if format_name == "json" then > + if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then > + error("log_format: 'json' can't be used with syslog logger") > + end > ffi.C.say_set_log_format(ffi.C.SF_JSON) > elseif format_name == "plain" then > ffi.C.say_set_log_format(ffi.C.SF_PLAIN) > diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua > index 0c11702c8..f1f323699 100755 > --- a/test/app-tap/logger.test.lua > +++ b/test/app-tap/logger.test.lua > @@ -1,13 +1,14 @@ > #!/usr/bin/env tarantool > > local test = require('tap').test('log') > -test:plan(24) > +test:plan(26) > > -- > -- Check that Tarantool creates ADMIN session for #! script > -- > local filename = "1.log" > local message = "Hello, World!" > + > box.cfg{ > log=filename, > memtx_memory=107374182, > @@ -121,5 +122,10 @@ line = line:sub(1, index) > message = json.decode(line) > test:is(message.message, "log file has been reopened", "check message after log.rotate()") > file:close() > + > +-- gh-3946: Check log_format usage. > +test:ok(pcall(require('log').log_format, "json"), "can used with boot log") > +test:ok(pcall(require('log').log_format, "plain"), "can used with boot log") > + This should go before box.cfg(), otherwise it tests nothing. Moved it. Pushed to master and 2.1. Here's the final patch: >From 6bcff2b5ffa48b96ff033cd1e0356c6dcba2eafc Mon Sep 17 00:00:00 2001 From: Roman Khabibov Date: Sat, 2 Mar 2019 20:12:11 +0300 Subject: [PATCH] say: fix assertion in log_format when called for boot or syslog logger It's OK to use json format with the boot logger. As for syslog, let's add a check to Lua's log_format() so that it fails gracefully rather than raising an assertion. Closes #3946 diff --git a/extra/exports b/extra/exports index d7242112..21fa9bf0 100644 --- a/extra/exports +++ b/extra/exports @@ -65,6 +65,7 @@ mp_encode_float mp_decode_double mp_decode_float +log_type say_set_log_level say_logrotate say_set_log_format diff --git a/src/lib/core/say.c b/src/lib/core/say.c index 50ee5569..68aa92f6 100644 --- a/src/lib/core/say.c +++ b/src/lib/core/say.c @@ -161,6 +161,12 @@ level_to_syslog_priority(int level) } } +enum say_logger_type +log_type() +{ + return log_default->type; +} + void log_set_level(struct log *log, enum say_level level) { @@ -171,9 +177,7 @@ void log_set_format(struct log *log, log_format_func_t format_func) { assert(format_func == say_format_plain || - log->type == SAY_LOGGER_STDERR || - log->type == SAY_LOGGER_PIPE || log->type == SAY_LOGGER_FILE); - + log->type != SAY_LOGGER_SYSLOG); log->format_func = format_func; } diff --git a/src/lib/core/say.h b/src/lib/core/say.h index 70050362..d26c3dde 100644 --- a/src/lib/core/say.h +++ b/src/lib/core/say.h @@ -196,6 +196,13 @@ log_say(struct log *log, int level, const char *filename, int line, const char *error, const char *format, ...); /** + * Default logger type info. + * @retval say_logger_type. + */ +enum say_logger_type +log_type(); + +/** * Set log level. Can be used dynamically. * * @param log log object diff --git a/src/lua/log.lua b/src/lua/log.lua index 0ac0e8f2..312c14d5 100644 --- a/src/lua/log.lua +++ b/src/lua/log.lua @@ -4,6 +4,18 @@ local ffi = require('ffi') ffi.cdef[[ typedef void (*sayfunc_t)(int level, const char *filename, int line, const char *error, const char *format, ...); + + enum say_logger_type { + SAY_LOGGER_BOOT, + SAY_LOGGER_STDERR, + SAY_LOGGER_FILE, + SAY_LOGGER_PIPE, + SAY_LOGGER_SYSLOG + }; + + enum say_logger_type + log_type(); + void say_set_log_level(int new_level); @@ -117,6 +129,9 @@ end local function log_format(format_name) if format_name == "json" then + if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then + error("log_format: 'json' can't be used with syslog logger") + end ffi.C.say_set_log_format(ffi.C.SF_JSON) elseif format_name == "plain" then ffi.C.say_set_log_format(ffi.C.SF_PLAIN) diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua index 0c11702c..492d5ea0 100755 --- a/test/app-tap/logger.test.lua +++ b/test/app-tap/logger.test.lua @@ -3,6 +3,11 @@ local test = require('tap').test('log') test:plan(24) +-- gh-3946: Assertion failure when using log_format() before box.cfg() +local log = require('log') +log.log_format('json') +log.log_format('plain') + -- -- Check that Tarantool creates ADMIN session for #! script -- @@ -12,7 +17,6 @@ box.cfg{ log=filename, memtx_memory=107374182, } -local log = require('log') local fio = require('fio') local json = require('json') local fiber = require('fiber')