From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <roman.habibov@tarantool.org>
Content-Type: text/plain;
charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [tarantool-patches] [PATCH] log: add missing assert into
log_set_format()
From: Roman Khabibov <roman.habibov@tarantool.org>
In-Reply-To: <20190319083008.f3qma6b4vbhfolyk@esperanza>
Date: Mon, 25 Mar 2019 16:03:54 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <BEFEF41E-30A9-4ABB-B242-769DBE658616@tarantool.org>
References: <20190302172946.94121-1-roman.habibov@tarantool.org>
<20190305142140.3mguirkwwsk3waho@esperanza>
<43D75107-60D1-4598-B342-634AC7296DA7@tarantool.org>
<20190319083008.f3qma6b4vbhfolyk@esperanza>
To: tarantool-patches@freelists.org
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
List-ID: <tarantool-patches.dev.tarantool.org>
Don=E2=80=99t read previous letter.
> On Mar 19, 2019, at 11:30 AM, Vladimir Davydov =
<vdavydov.dev@gmail.com> wrote:
>=20
> 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
>>=20
>> log: throw errors when "json" with syslog or boot log
>=20
> Nit: the subsystem is called 'say' so the subject line should be
>=20
> say: =E2=80=A6
>>=20
>> Add function returning logger info to throw errors from lua when =
"json" used with syslog or boot log.
>=20
> Nit: please keep the commit message text width within 72 symbols:
>=20
> 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
=20
Add function returning logger type to throw error from lua
when "json" used with syslog.
>>=20
>> Closes #3946
>>=20
>> 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
>>=20
>> +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)
>> }
>> }
>>=20
>> +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, ...);
>>=20
>> +/**
>> + * Default logger type info.
>> + * @retval say_logger_type.
>> + */
>> +enum say_logger_type
>> +log_default_info();
>> +
>=20
> 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 =3D 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);
>>=20
>> - void
>> + int
>> say_set_log_format(enum say_format format);
>>=20
>>=20
>> @@ -117,6 +129,11 @@ end
>>=20
>> local function log_format(format_name)
>> if format_name =3D=3D "json" then
>> + if ffi.C.log_default_info() =3D=3D ffi.C.SAY_LOGGER_BOOT =
then
>> + error("log_format: 'json' can not used with boot log")
>=20
> 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 =3D=3D say_format_plain ||
+ assert(format_func =3D=3D say_format_json ||
+ format_func =3D=3D say_format_plain ||
log->type =3D=3D SAY_LOGGER_STDERR ||
log->type =3D=3D SAY_LOGGER_PIPE || log->type =3D=3D =
SAY_LOGGER_FILE);
>> + elseif ffi.C.log_default_info() =3D=3D =
ffi.C.SAY_LOGGER_SYSLOG then
>> + error("log_format: 'json' can not used with syslog")
>=20
> Let's please make this message match the one printed by box_check_say:
>=20
> log_format: 'json' can't be used with syslog logger
+ if ffi.C.log_type() =3D=3D ffi.C.SAY_LOGGER_SYSLOG then
+ error("log_format: 'json' can't be used with syslog =
logger")
+ end
>=20
>> + end
>> ffi.C.say_set_log_format(ffi.C.SF_JSON)
>> elseif format_name =3D=3D "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
>>=20
>> local test =3D require('tap').test('log')
>> -test:plan(24)
>> +test:plan(25)
>>=20
>> --
>> -- Check that Tarantool creates ADMIN session for #! script
>> --
>> local filename =3D "1.log"
>> local message =3D "Hello, World!"
>> +
>> +-- Check log_format("json") usage before box.cfg{}.
>=20
> We typically add ticket number (gh-####) to the comment to a test =
case.
>=20
>> +test:ok(not pcall(require('log').log_format, "json"), "can not used =
with boot log")
>> +
>=20
> 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 =3D 'syslog:identity=3Dtarantool=E2=80=99} 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
=20
Add function returning logger type to throw error from lua
when "json" used with syslog.
=20
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
=20
+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)
}
}
=20
+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 =3D=3D say_format_plain ||
+ assert(format_func =3D=3D say_format_json ||
+ format_func =3D=3D say_format_plain ||
log->type =3D=3D SAY_LOGGER_STDERR ||
log->type =3D=3D SAY_LOGGER_PIPE || log->type =3D=3D =
SAY_LOGGER_FILE);
=20
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, ...);
=20
+/**
+ * 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 =3D 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);
=20
- void
+ int
say_set_log_format(enum say_format format);
=20
=20
@@ -117,6 +129,9 @@ end
=20
local function log_format(format_name)
if format_name =3D=3D "json" then
+ if ffi.C.log_type() =3D=3D 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 =3D=3D "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
=20
local test =3D require('tap').test('log')
-test:plan(24)
+test:plan(26)
=20
--
-- Check that Tarantool creates ADMIN session for #! script
--
local filename =3D "1.log"
local message =3D "Hello, World!"
+
box.cfg{
log=3Dfilename,
memtx_memory=3D107374182,
@@ -121,5 +122,10 @@ line =3D line:sub(1, index)
message =3D 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()