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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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