[Tarantool-patches] [PATCH] say: fix syslog format

[Igor Munkin]

Olya,

Thanks for the patch! I left several comments below, please consider
them.

On 24.03.20, Olga Arkhangelskaia wrote:
> While refactoring of say module in commit 2.0.4-837-g5db765a76

AFAICS, we mention commits the following way:
| * b5b4809cf2e6d48230eb9e4301eac188b080e0f4 ('replication: update
| replica gc state on subscribe');

> format of syslog was broken.
> 
> Closes #4785
> ---
>  src/lib/core/say.c                   |  6 +++++-
>  test/box-tap/gh-4785-syslog.result   |  1 +
>  test/box-tap/gh-4785-syslog.test.lua | 22 ++++++++++++++++++++++
>  3 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 test/box-tap/gh-4785-syslog.result
>  create mode 100755 test/box-tap/gh-4785-syslog.test.lua
> 
> diff --git a/src/lib/core/say.c b/src/lib/core/say.c
> index dd05285a6..d03e687b5 100644
> --- a/src/lib/core/say.c
> +++ b/src/lib/core/say.c
> @@ -199,7 +199,11 @@ void
>  say_set_log_format(enum say_format format)
>  {
>  	log_format_func_t format_func;
> -
> +	bool allowed_to_change = log_default->type == SAY_LOGGER_STDERR ||
> +			         log_default->type == SAY_LOGGER_PIPE ||
> +				 log_default->type == SAY_LOGGER_FILE;

Considering the values in SAY_LOGGER_* enum only two types are not
allowed to be changed: SAY_LOGGER_SYSLOG (since syslog format is fixed)
and SAY_LOGGER_BOOT (and I have no idea why). Thereby it would be great
to mention unchangable logger types with the corresponding reasons.
Also, considering the reasons the inverted condition looks more
convenient.

> +	if (!allowed_to_change)
> +		return;
>  	switch (format) {
>  	case SF_JSON:
>  		assert(log_default->type != SAY_LOGGER_SYSLOG);

This assertion never raises after the patch, so can be dropped.

> diff --git a/test/box-tap/gh-4785-syslog.result b/test/box-tap/gh-4785-syslog.result
> new file mode 100644
> index 000000000..bf3df9e6e
> --- /dev/null
> +++ b/test/box-tap/gh-4785-syslog.result
> @@ -0,0 +1 @@
> +<>TIMESTAMP tarantool[PID]: main//gh.test.lua C> Tarantool 
> diff --git a/test/box-tap/gh-4785-syslog.test.lua b/test/box-tap/gh-4785-syslog.test.lua
> new file mode 100755
> index 000000000..586a2eb1a
> --- /dev/null
> +++ b/test/box-tap/gh-4785-syslog.test.lua

Since you created a diff-based test it has to be moved to box directory
otherwise you can leave it here and transform it to a TAP-based one.
I propose to go the latter way.

> @@ -0,0 +1,22 @@
> +#!/usr/bin/env tarantool
> +test_run = require('test_run').new()

Seems like you don't need 'test_run' object here.

> +
> +local socket = require('socket')
> +local log = require('log')
> +local fio = require('fio')
> +
> +path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
> +unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
> +unix_socket:bind('unix/', path)
> +
> +opt = string.format("syslog:server=unix:%s,identity=tarantool", path)
> +local buf = ''
> +box.cfg{log = opt}
> +buf = buf .. unix_socket:recv(100)
> +str = string.format(buf)
> +-- Check syslog format: TIMESTAMP IDENTATION[PID]
> +--<>TIMESTAMP tarantool[PID]: main//gh.test.lua C> Tarantool
> +print(((((str:gsub("%d", "")):gsub("%p%p--%w+", "")):gsub("%w+  ::", "TIMESTAMP")):gsub("]", "PID]")))

You can just use test:like here to check the received log entry. Also
<buf> and <opt> variables are excess and parentheses can be omitted.
Furthermore, syslog entry is a well documented format[1], so you can
match it against an RFC pattern. I reimplemented the test the following
way (the diff is below):

================================================================================

diff --git a/test/box-tap/gh-4785-syslog.result
b/test/box-tap/gh-4785-syslog.result
deleted file mode 100644
index bf3df9e6e..000000000
--- a/test/box-tap/gh-4785-syslog.result
+++ /dev/null
@@ -1 +0,0 @@
-<>TIMESTAMP tarantool[PID]: main//gh.test.lua C> Tarantool 
diff --git a/test/box-tap/gh-4785-syslog.test.lua b/test/box-tap/gh-4785-syslog.test.lua
index 586a2eb1a..bdf8d5d88 100755
--- a/test/box-tap/gh-4785-syslog.test.lua
+++ b/test/box-tap/gh-4785-syslog.test.lua
@@ -1,22 +1,39 @@
 #!/usr/bin/env tarantool
-test_run = require('test_run').new()
-
 local socket = require('socket')
 local log = require('log')
 local fio = require('fio')
+local tap = require('tap')
+
+local test = tap.test("Tarantool 4785")
+test:plan(1)
 
-path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
-unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
+local identity = 'tarantool'
+local path = fio.pathjoin(fio.cwd(), 'log_unix_socket_test.sock')
+local unix_socket = socket('AF_UNIX', 'SOCK_DGRAM', 0)
 unix_socket:bind('unix/', path)

-opt = string.format("syslog:server=unix:%s,identity=tarantool", path)
-local buf = ''
-box.cfg{log = opt}
-buf = buf .. unix_socket:recv(100)
-str = string.format(buf)
--- Check syslog format: TIMESTAMP IDENTATION[PID]
---<>TIMESTAMP tarantool[PID]: main//gh.test.lua C> Tarantool
-print(((((str:gsub("%d", "")):gsub("%p%p--%w+", "")):gsub("%w+  ::", "TIMESTAMP")):gsub("]", "PID]")))
+box.cfg{
+  log = string.format("syslog:server=unix:%s,identity=%s", path, identity)
+}
+
+local entry = unix_socket:recv(100)
+-- Check syslog format: <PRI><TIMESTAMP> IDENTITY[PID]: CORD/FID/FILE FACILITY>
+local patterns = {
+  PRI       = '<%d+>',
+  TIMESTAMP = '%u%l%l  ?%d?%d %d%d:%d%d:%d%d',
+  IDENTITY  = identity,
+  PID       = '%[%d+%]',
+  CORD      = 'main',
+  FID       = '%d+',
+  FILE      = '.+%.lua',
+  FACILITY  = '%u'
+}
+
+local pattern = string.format('%s%s %s%s: %s/%s/%s %s>',
+                              patterns.PRI, patterns.TIMESTAMP,
+                              patterns.IDENTITY, patterns.PID, patterns.CORD,
+                              patterns.FID, patterns.FILE, patterns.FACILITY)
+test:like(entry, pattern, 'syslog format')
 unix_socket:close()
 os.remove(path)
-os.exit(0)
+os.exit(test:check() and 0 or 1)

================================================================================

> +unix_socket:close()
> +os.remove(path)
> +os.exit(0)
> -- 
> 2.20.1 (Apple Git-117)
> 

[1]: https://tools.ietf.org/html/rfc3164#section-4.1

-- 
Best regards,
IM