* [tarantool-patches] [PATCH] log: add missing assert into log_set_format() @ 2019-03-02 17:29 Roman Khabibov 2019-03-05 14:21 ` Vladimir Davydov 0 siblings, 1 reply; 7+ messages in thread From: Roman Khabibov @ 2019-03-02 17:29 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Add missing condition for json format into assert in log_set_format() function. Closes #3946 --- Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3946-log-assert/test Issue: https://github.com/tarantool/tarantool/issues/3946 src/lib/core/say.c | 3 ++- test/app-tap/logger.test.lua | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lib/core/say.c b/src/lib/core/say.c index 50ee55692..758b1c234 100644 --- a/src/lib/core/say.c +++ b/src/lib/core/say.c @@ -170,7 +170,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); diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua index 0c11702c8..4e18d53e9 100755 --- a/test/app-tap/logger.test.lua +++ b/test/app-tap/logger.test.lua @@ -1,13 +1,17 @@ #!/usr/bin/env tarantool local test = require('tap').test('log') -test:plan(24) +test:plan(25) -- -- Check that Tarantool creates ADMIN session for #! script -- local filename = "1.log" local message = "Hello, World!" + +jlog = require('log') +test:ok(jlog.log_format("json") == nil, "no assertions") + box.cfg{ log=filename, memtx_memory=107374182, -- 2.17.2 (Apple Git-113) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] [PATCH] log: add missing assert into log_set_format() 2019-03-02 17:29 [tarantool-patches] [PATCH] log: add missing assert into log_set_format() Roman Khabibov @ 2019-03-05 14:21 ` Vladimir Davydov 2019-03-18 15:35 ` [tarantool-patches] " Roman Khabibov 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Davydov @ 2019-03-05 14:21 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches, v.shpilevoy On Sat, Mar 02, 2019 at 08:29:46PM +0300, Roman Khabibov wrote: > Add missing condition for json format into assert in log_set_format() function. > > Closes #3946 > --- > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3946-log-assert/test > Issue: https://github.com/tarantool/tarantool/issues/3946 > > src/lib/core/say.c | 3 ++- > test/app-tap/logger.test.lua | 6 +++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/lib/core/say.c b/src/lib/core/say.c > index 50ee55692..758b1c234 100644 > --- a/src/lib/core/say.c > +++ b/src/lib/core/say.c > @@ -170,7 +170,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); The assertion is meaningless after this change. At least the log->type check. I guess the idea was to check that json format isn't used in conjunction with syslog. Also, there's another similar assertion failure that I think should be fixed in the scope of this patch: box.cfg{log = 'syslog:identity=tarantool'} require('log').log_format('json') tarantool: /home/vlad/src/tarantool/src/lib/core/say.c:195: say_set_log_format: Assertion `log_default->type != SAY_LOGGER_SYSLOG' failed. > > diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua > index 0c11702c8..4e18d53e9 100755 > --- a/test/app-tap/logger.test.lua > +++ b/test/app-tap/logger.test.lua > @@ -1,13 +1,17 @@ > #!/usr/bin/env tarantool > > local test = require('tap').test('log') > -test:plan(24) > +test:plan(25) > > -- > -- Check that Tarantool creates ADMIN session for #! script > -- > local filename = "1.log" > local message = "Hello, World!" > + > +jlog = require('log') > +test:ok(jlog.log_format("json") == nil, "no assertions") > + Squeezing a test case like this, without a proper comment, looks bad - it seems that the comment above is related to it although it isn't. Also, jlog is a bad name. If you don't want to pollute the namespace, you can get along without it: require('log').log_format('json') > box.cfg{ > log=filename, > memtx_memory=107374182, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] Re: [PATCH] log: add missing assert into log_set_format() 2019-03-05 14:21 ` Vladimir Davydov @ 2019-03-18 15:35 ` Roman Khabibov 2019-03-19 8:30 ` Vladimir Davydov 0 siblings, 1 reply; 7+ messages in thread From: Roman Khabibov @ 2019-03-18 15:35 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladimir Davydov Hi! Thanks for review. > On Mar 5, 2019, at 5:21 PM, Vladimir Davydov <vdavydov.dev@gmail.com> wrote: > > On Sat, Mar 02, 2019 at 08:29:46PM +0300, Roman Khabibov wrote: >> Add missing condition for json format into assert in log_set_format() function. >> >> Closes #3946 >> --- >> >> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3946-log-assert/test >> Issue: https://github.com/tarantool/tarantool/issues/3946 >> >> src/lib/core/say.c | 3 ++- >> test/app-tap/logger.test.lua | 6 +++++- >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >> index 50ee55692..758b1c234 100644 >> --- a/src/lib/core/say.c >> +++ b/src/lib/core/say.c >> @@ -170,7 +170,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); > > The assertion is meaningless after this change. At least the log->type > check. I guess the idea was to check that json format isn't used in > conjunction with syslog. > > Also, there's another similar assertion failure that I think should be > fixed in the scope of this patch: > > box.cfg{log = 'syslog:identity=tarantool'} > require('log').log_format('json') > > tarantool: /home/vlad/src/tarantool/src/lib/core/say.c:195: say_set_log_format: Assertion `log_default->type != SAY_LOGGER_SYSLOG' failed. Redone. >> >> diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua >> index 0c11702c8..4e18d53e9 100755 >> --- a/test/app-tap/logger.test.lua >> +++ b/test/app-tap/logger.test.lua >> @@ -1,13 +1,17 @@ >> #!/usr/bin/env tarantool >> >> local test = require('tap').test('log') >> -test:plan(24) >> +test:plan(25) >> >> -- >> -- Check that Tarantool creates ADMIN session for #! script >> -- >> local filename = "1.log" >> local message = "Hello, World!" >> + >> +jlog = require('log') >> +test:ok(jlog.log_format("json") == nil, "no assertions") >> + > > Squeezing a test case like this, without a proper comment, looks bad - > it seems that the comment above is related to it although it isn't. > Also, jlog is a bad name. If you don't want to pollute the namespace, > you can get along without it: > > require('log').log_format('json') > >> box.cfg{ >> log=filename, >> memtx_memory=107374182, Redone. + +-- Check log_format("json") usage before box.cfg{}. +test:ok(not pcall(require('log').log_format, "json"), "can not used with boot log") + commit d2cec226e903c8ecf3f1e697fd48f617f92ce27f Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Sat Mar 2 20:12:11 2019 +0300 log: throw errors when "json" with syslog or boot log Add function returning logger info to throw errors from lua when "json" used with syslog or boot log. Closes #3946 diff --git a/extra/exports b/extra/exports index d72421123..953513845 100644 --- a/extra/exports +++ b/extra/exports @@ -65,6 +65,7 @@ mp_encode_float mp_decode_double mp_decode_float +log_default_info 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..936f95160 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_default_info() +{ + return log_default->type; +} + void log_set_level(struct log *log, enum say_level level) { diff --git a/src/lib/core/say.h b/src/lib/core/say.h index 70050362c..829487a5b 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_default_info(); + /** * Set log level. Can be used dynamically. * diff --git a/src/lua/log.lua b/src/lua/log.lua index 0ac0e8f26..0b084a3e7 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_default_info(); + void say_set_log_level(int new_level); - void + int say_set_log_format(enum say_format format); @@ -117,6 +129,11 @@ end local function log_format(format_name) if format_name == "json" then + if ffi.C.log_default_info() == ffi.C.SAY_LOGGER_BOOT then + error("log_format: 'json' can not used with boot log") + elseif ffi.C.log_default_info() == ffi.C.SAY_LOGGER_SYSLOG then + error("log_format: 'json' can not used with syslog") + 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..abcb9044b 100755 --- a/test/app-tap/logger.test.lua +++ b/test/app-tap/logger.test.lua @@ -1,13 +1,17 @@ #!/usr/bin/env tarantool local test = require('tap').test('log') -test:plan(24) +test:plan(25) -- -- Check that Tarantool creates ADMIN session for #! script -- local filename = "1.log" local message = "Hello, World!" + +-- Check log_format("json") usage before box.cfg{}. +test:ok(not pcall(require('log').log_format, "json"), "can not used with boot log") + box.cfg{ log=filename, memtx_memory=107374182, ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] Re: [PATCH] log: add missing assert into log_set_format() 2019-03-18 15:35 ` [tarantool-patches] " Roman Khabibov @ 2019-03-19 8:30 ` Vladimir Davydov 2019-03-25 12:43 ` [tarantool-patches] " Roman Khabibov 2019-03-25 13:03 ` Roman Khabibov 0 siblings, 2 replies; 7+ messages in thread From: Vladimir Davydov @ 2019-03-19 8:30 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On Mon, Mar 18, 2019 at 06:35:55PM +0300, Roman Khabibov wrote: > commit d2cec226e903c8ecf3f1e697fd48f617f92ce27f > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Sat Mar 2 20:12:11 2019 +0300 > > log: throw errors when "json" with syslog or boot log Nit: the subsystem is called 'say' so the subject line should be say: ... > > Add function returning logger info to throw errors from lua when "json" used with syslog or boot log. Nit: please keep the commit message text width within 72 symbols: Add function returning logger info to throw errors from lua when "json" used with syslog or boot log. > > Closes #3946 > > diff --git a/extra/exports b/extra/exports > index d72421123..953513845 100644 > --- a/extra/exports > +++ b/extra/exports > @@ -65,6 +65,7 @@ mp_encode_float > mp_decode_double > mp_decode_float > > +log_default_info > 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..936f95160 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_default_info() > +{ > + return log_default->type; > +} > + > void > log_set_level(struct log *log, enum say_level level) > { > diff --git a/src/lib/core/say.h b/src/lib/core/say.h > index 70050362c..829487a5b 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_default_info(); > + Bad name. What's 'info'? What's 'default'? None of the words is used anywhere else in say.h. Let's call it simply log_type, may be? That would match log_level and log_format global variables. > /** > * Set log level. Can be used dynamically. > * > diff --git a/src/lua/log.lua b/src/lua/log.lua > index 0ac0e8f26..0b084a3e7 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_default_info(); > + > void > say_set_log_level(int new_level); > > - void > + int > say_set_log_format(enum say_format format); > > > @@ -117,6 +129,11 @@ end > > local function log_format(format_name) > if format_name == "json" then > + if ffi.C.log_default_info() == ffi.C.SAY_LOGGER_BOOT then > + error("log_format: 'json' can not used with boot log") I don't think we need to prohibit using 'json' with the boot logger. After all, it's just like the stderr logger. Besides, we allow to set 'plain' format for the boot logger, which enriches the output in its own way. That said, let's raise an error only on attempt to use 'json' format with 'syslog' logger, just like we do in box_check_say. > + elseif ffi.C.log_default_info() == ffi.C.SAY_LOGGER_SYSLOG then > + error("log_format: 'json' can not used with syslog") Let's please make this message match the one printed by box_check_say: 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..abcb9044b 100755 > --- a/test/app-tap/logger.test.lua > +++ b/test/app-tap/logger.test.lua > @@ -1,13 +1,17 @@ > #!/usr/bin/env tarantool > > local test = require('tap').test('log') > -test:plan(24) > +test:plan(25) > > -- > -- Check that Tarantool creates ADMIN session for #! script > -- > local filename = "1.log" > local message = "Hello, World!" > + > +-- Check log_format("json") usage before box.cfg{}. We typically add ticket number (gh-####) to the comment to a test case. > +test:ok(not pcall(require('log').log_format, "json"), "can not used with boot log") > + You added a new test case in the middle of another one (the comment above is for box.cfg below). Please don't do that - this makes tests difficult to maintain - move it above. Also, please check that setting 'plain' format works as well. > box.cfg{ > log=filename, > memtx_memory=107374182, > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] [PATCH] log: add missing assert into log_set_format() 2019-03-19 8:30 ` Vladimir Davydov @ 2019-03-25 12:43 ` Roman Khabibov 2019-03-25 13:03 ` Roman Khabibov 1 sibling, 0 replies; 7+ messages in thread From: Roman Khabibov @ 2019-03-25 12:43 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladimir Davydov Hi! Thanks for review. > On Mar 19, 2019, at 11:30 AM, Vladimir Davydov <vdavydov.dev@gmail.com> wrote: > > On Mon, Mar 18, 2019 at 06:35:55PM +0300, Roman Khabibov wrote: >> commit d2cec226e903c8ecf3f1e697fd48f617f92ce27f >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Sat Mar 2 20:12:11 2019 +0300 >> >> log: throw errors when "json" with syslog or boot log > > Nit: the subsystem is called 'say' so the subject line should be > > say: … >> >> Add function returning logger info to throw errors from lua when "json" used with syslog or boot log. > > Nit: please keep the commit message text width within 72 symbols: > > Add function returning logger info to throw errors from lua when "json" > used with syslog or boot log. say: throw errors when "json" with syslog or boot log Add function returning logger type to throw error from lua when "json" used with syslog. >> >> Closes #3946 >> >> diff --git a/extra/exports b/extra/exports >> index d72421123..953513845 100644 >> --- a/extra/exports >> +++ b/extra/exports >> @@ -65,6 +65,7 @@ mp_encode_float >> mp_decode_double >> mp_decode_float >> >> +log_default_info >> 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..936f95160 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_default_info() >> +{ >> + return log_default->type; >> +} >> + >> void >> log_set_level(struct log *log, enum say_level level) >> { >> diff --git a/src/lib/core/say.h b/src/lib/core/say.h >> index 70050362c..829487a5b 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_default_info(); >> + > > Bad name. What's 'info'? What's 'default'? None of the words is used > anywhere else in say.h. Let's call it simply log_type, may be? That > would match log_level and log_format global variables. +enum say_logger_type +log_type() +{ + return log_default->type; +} + >> /** >> * Set log level. Can be used dynamically. >> * >> diff --git a/src/lua/log.lua b/src/lua/log.lua >> index 0ac0e8f26..0b084a3e7 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_default_info(); >> + >> void >> say_set_log_level(int new_level); >> >> - void >> + int >> say_set_log_format(enum say_format format); >> >> >> @@ -117,6 +129,11 @@ end >> >> local function log_format(format_name) >> if format_name == "json" then >> + if ffi.C.log_default_info() == ffi.C.SAY_LOGGER_BOOT then >> + error("log_format: 'json' can not used with boot log") > > I don't think we need to prohibit using 'json' with the boot logger. > After all, it's just like the stderr logger. Besides, we allow to set > 'plain' format for the boot logger, which enriches the output in its > own way. That said, let's raise an error only on attempt to use 'json' > format with 'syslog' logger, just like we do in box_check_say. Then I return previous variant of assert. 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); >> + elseif ffi.C.log_default_info() == ffi.C.SAY_LOGGER_SYSLOG then >> + error("log_format: 'json' can not used with syslog") > > Let's please make this message match the one printed by box_check_say: > > log_format: 'json' can't be used with syslog logger + if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then + error("log_format: 'json' can't be used with syslog logger") + end > >> + 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..abcb9044b 100755 >> --- a/test/app-tap/logger.test.lua >> +++ b/test/app-tap/logger.test.lua >> @@ -1,13 +1,17 @@ >> #!/usr/bin/env tarantool >> >> local test = require('tap').test('log') >> -test:plan(24) >> +test:plan(25) >> >> -- >> -- Check that Tarantool creates ADMIN session for #! script >> -- >> local filename = "1.log" >> local message = "Hello, World!" >> + >> +-- Check log_format("json") usage before box.cfg{}. > > We typically add ticket number (gh-####) to the comment to a test case. > >> +test:ok(not pcall(require('log').log_format, "json"), "can not used with boot log") >> + > > You added a new test case in the middle of another one (the comment > above is for box.cfg below). Please don't do that - this makes tests > difficult to maintain - move it above. + +-- 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") + > Also, please check that setting 'plain' format works as well. How to call box.cfg{log = 'syslog:identity=tarantool’} in this test file when it called already with other parameters? Can we restart server in the tap tests? commit d6d4d7021bf418928bb728594456c5305426b2e6 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Sat Mar 2 20:12:11 2019 +0300 say: throw errors when "json" with syslog or boot log Add function returning logger type to throw error from lua when "json" used with syslog. Closes #3946 diff --git a/extra/exports b/extra/exports index d72421123..953513845 100644 --- a/extra/exports +++ b/extra/exports @@ -65,6 +65,7 @@ mp_encode_float mp_decode_double mp_decode_float +log_default_info 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); 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 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") + test:check() os.exit() ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] [PATCH] log: add missing assert into log_set_format() 2019-03-19 8:30 ` Vladimir Davydov 2019-03-25 12:43 ` [tarantool-patches] " Roman Khabibov @ 2019-03-25 13:03 ` Roman Khabibov 2019-03-26 17:26 ` Vladimir Davydov 1 sibling, 1 reply; 7+ messages in thread From: Roman Khabibov @ 2019-03-25 13:03 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladimir Davydov Don’t read previous letter. > On Mar 19, 2019, at 11:30 AM, Vladimir Davydov <vdavydov.dev@gmail.com> wrote: > > On Mon, Mar 18, 2019 at 06:35:55PM +0300, Roman Khabibov wrote: >> commit d2cec226e903c8ecf3f1e697fd48f617f92ce27f >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Sat Mar 2 20:12:11 2019 +0300 >> >> log: throw errors when "json" with syslog or boot log > > Nit: the subsystem is called 'say' so the subject line should be > > say: … >> >> Add function returning logger info to throw errors from lua when "json" used with syslog or boot log. > > Nit: please keep the commit message text width within 72 symbols: > > Add function returning logger info to throw errors from lua when "json" > used with syslog or boot log. 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 >> >> diff --git a/extra/exports b/extra/exports >> index d72421123..953513845 100644 >> --- a/extra/exports >> +++ b/extra/exports >> @@ -65,6 +65,7 @@ mp_encode_float >> mp_decode_double >> mp_decode_float >> >> +log_default_info >> 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..936f95160 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_default_info() >> +{ >> + return log_default->type; >> +} >> + >> void >> log_set_level(struct log *log, enum say_level level) >> { >> diff --git a/src/lib/core/say.h b/src/lib/core/say.h >> index 70050362c..829487a5b 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_default_info(); >> + > > Bad name. What's 'info'? What's 'default'? None of the words is used > anywhere else in say.h. Let's call it simply log_type, may be? That > would match log_level and log_format global variables. +enum say_logger_type +log_type() +{ + return log_default->type; +} + >> /** >> * Set log level. Can be used dynamically. >> * >> diff --git a/src/lua/log.lua b/src/lua/log.lua >> index 0ac0e8f26..0b084a3e7 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_default_info(); >> + >> void >> say_set_log_level(int new_level); >> >> - void >> + int >> say_set_log_format(enum say_format format); >> >> >> @@ -117,6 +129,11 @@ end >> >> local function log_format(format_name) >> if format_name == "json" then >> + if ffi.C.log_default_info() == ffi.C.SAY_LOGGER_BOOT then >> + error("log_format: 'json' can not used with boot log") > > I don't think we need to prohibit using 'json' with the boot logger. > After all, it's just like the stderr logger. Besides, we allow to set > 'plain' format for the boot logger, which enriches the output in its > own way. That said, let's raise an error only on attempt to use 'json' > format with 'syslog' logger, just like we do in box_check_say. Then I return previous variant of assert. 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); >> + elseif ffi.C.log_default_info() == ffi.C.SAY_LOGGER_SYSLOG then >> + error("log_format: 'json' can not used with syslog") > > Let's please make this message match the one printed by box_check_say: > > log_format: 'json' can't be used with syslog logger + if ffi.C.log_type() == ffi.C.SAY_LOGGER_SYSLOG then + error("log_format: 'json' can't be used with syslog logger") + end > >> + 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..abcb9044b 100755 >> --- a/test/app-tap/logger.test.lua >> +++ b/test/app-tap/logger.test.lua >> @@ -1,13 +1,17 @@ >> #!/usr/bin/env tarantool >> >> local test = require('tap').test('log') >> -test:plan(24) >> +test:plan(25) >> >> -- >> -- Check that Tarantool creates ADMIN session for #! script >> -- >> local filename = "1.log" >> local message = "Hello, World!" >> + >> +-- Check log_format("json") usage before box.cfg{}. > > We typically add ticket number (gh-####) to the comment to a test case. > >> +test:ok(not pcall(require('log').log_format, "json"), "can not used with boot log") >> + > > You added a new test case in the middle of another one (the comment > above is for box.cfg below). Please don't do that - this makes tests > difficult to maintain - move it above. + +-- 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") + > Also, please check that setting 'plain' format works as well. How to call box.cfg{log = 'syslog:identity=tarantool’} in this test file when it called already with other parameters? Can we restart server in the tap tests? commit 4bc90124d0cb27f8eec4a0e1d656fcbb35b919dc Author: Roman Khabibov <roman.habibov@tarantool.org> 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 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); 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 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") + test:check() os.exit() ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [tarantool-patches] [PATCH] log: add missing assert into log_set_format() 2019-03-25 13:03 ` Roman Khabibov @ 2019-03-26 17:26 ` Vladimir Davydov 0 siblings, 0 replies; 7+ messages in thread From: Vladimir Davydov @ 2019-03-26 17:26 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches On Mon, Mar 25, 2019 at 04:03:54PM +0300, Roman Khabibov wrote: > commit 4bc90124d0cb27f8eec4a0e1d656fcbb35b919dc > Author: Roman Khabibov <roman.habibov@tarantool.org> > 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 <roman.habibov@tarantool.org> 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') ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-26 17:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-02 17:29 [tarantool-patches] [PATCH] log: add missing assert into log_set_format() Roman Khabibov 2019-03-05 14:21 ` Vladimir Davydov 2019-03-18 15:35 ` [tarantool-patches] " Roman Khabibov 2019-03-19 8:30 ` Vladimir Davydov 2019-03-25 12:43 ` [tarantool-patches] " Roman Khabibov 2019-03-25 13:03 ` Roman Khabibov 2019-03-26 17:26 ` Vladimir Davydov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox