From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 8CFEC4696C3 for ; Wed, 15 Apr 2020 16:25:29 +0300 (MSK) References: <20200324085243.29337-1-arkholga@tarantool.org> <20200414125207.GK5713@tarantool.org> From: Olga Arkhangelskaia Message-ID: <67f56383-7f34-853b-39b7-f4e04dfed595@tarantool.org> Date: Wed, 15 Apr 2020 16:25:28 +0300 MIME-Version: 1.0 In-Reply-To: <20200414125207.GK5713@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi Igor! Thanks for the review! I have fixed the patch according to what we have discussed. Please, see diff below 14.04.2020 15:52, 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 > 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  While refactoring of say module in commit   7 5db765a76ab7270b350960a2dd6d89dcd3bdc032 (say: fix non-informative error   8 messages for log cfg) format of syslog was broken.  30 +`....../* For syslog, default or boot log type the log format can  31 +`...... * not be changed.  32 +`...... */  33 +`......bool unchangeable =  log_default->type == SAY_LOGGER_BOOT ||  34 +`......`.......`.......     log_default->type == SAY_LOGGER_SYSLOG;  35 +`......if (unchangeable)  36 +`......`.......return;  37  `......switch (format) {  38  `......case SF_JSON:  39 -`......`.......assert(log_default->type != SAY_LOGGER_SYSLOG);  40  `......`.......format_func = say_format_json;  41 `......`.......break;  42  `......case SF_PLAIN: >