[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 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.

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