From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 423C045C316 for ; Tue, 14 Apr 2020 15:59:14 +0300 (MSK) Date: Tue, 14 Apr 2020 15:52:08 +0300 From: Igor Munkin Message-ID: <20200414125207.GK5713@tarantool.org> References: <20200324085243.29337-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200324085243.29337-1-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] say: fix syslog format List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org 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 and 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: 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