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

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

[Roman Khabibov]

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)

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

[Vladimir Davydov]

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,

Re: [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@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,

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

[Vladimir Davydov]

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,
> 

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

[Roman Khabibov]

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()

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

[Roman Khabibov]

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()

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

[Vladimir Davydov]

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')