[tarantool-patches] [PATCH] log: add missing assert into log_set_format()
Roman Khabibov
roman.habibov at tarantool.org
Mon Mar 25 16:03:54 MSK 2019
Don’t read previous letter.
> On Mar 19, 2019, at 11:30 AM, Vladimir Davydov <vdavydov.dev at gmail.com> wrote:
>
> On Mon, Mar 18, 2019 at 06:35:55PM +0300, Roman Khabibov wrote:
>> commit d2cec226e903c8ecf3f1e697fd48f617f92ce27f
>> Author: Roman Khabibov <roman.habibov at 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 at 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()
More information about the Tarantool-patches
mailing list