[tarantool-patches] Re: [PATCH] log: add missing assert into log_set_format()

[Roman Khabibov]

Hi! Thanks for review.

> On Mar 5, 2019, at 5:21 PM, Vladimir Davydov <vdavydov.dev at 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 at 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,