[patches] log rotation message patch

Konstantin Osipov kostja at tarantool.org
Tue Jan 30 22:38:21 MSK 2018


Hi,

Side note: I don't see this patch anywhere. It's not in
commits@, not in patches at . 

Plesae find my comments below.

There is a reason why we don't use log_say in log rotate. There is
even a comment just a few lines above of your change: 

/*                                                                      
 * The whole charade's purpose is to avoid log->fd changing.            
 * Remember, we are a signal handler.                                   
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 */  

Yes, again, log_rotate is called from a signal handler, and log_say,
which calls vsay, is not async-signal-safe. Please,
read up on async-signal-safety before writing any more code for
tarantool - this is a fundamental part of POSIX programming which
you need to keep in mind. You apparently still mix it up with
thread safety.

This patch also indicates a wrong mindset when working on a bug. 
You did not know why the syscall was there in the first place. And
you didn't care to investigate, you just "fixed" it. 
You should never, "fix" code you don't understand. Reviewers will not
catch your errors or do your job -- they will simply put your
patches on hold and will be right to do so. If you *do*, however,
understand, then make all the context clear to the reviewer. If you 
assume there is a dependency this patch and the patch which makes
the log rotation handler signal-safe - which I think is a dangerous idea
- please write this in a changeset comment.

A final remark is about the test case. Please avoid using POSIX specific
command line tools, such as mv, in tests. Use fio instead. We still may 
need to port to Windows, and in this case I don't want us to rewrite
the entire test suite.

diff --git a/src/say.c b/src/say.c
index 65808a6..d095280 100644
--- a/src/say.c
+++ b/src/say.c
@@ -236,7 +236,7 @@ write_to_syslog(struct log *log, int total);
  * Rotate logs on SIGHUP
  */
 static int
-log_rotate(const struct log *log)
+log_rotate(struct log *log)
 {
 	if (log->type != SAY_LOGGER_FILE) {
 		return 0;
@@ -261,10 +261,8 @@ log_rotate(const struct log *log)
 			say_syserror("fcntl, fd=%i", log->fd);
 		}
 	}
-	char logrotate_message[] = "log file has been reopened\n";
-	ssize_t r = write(log->fd,
-			  logrotate_message, (sizeof logrotate_message) - 1);
-	(void) r;
+	log_say(log, S_INFO, __FILE__, __LINE__, NULL,
+		"log file has been reopened");
 	return 0;
 }
 
diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
index 2d0f333..3933401 100755
--- a/test/app-tap/logger.test.lua
+++ b/test/app-tap/logger.test.lua
@@ -1,7 +1,7 @@
 #!/usr/bin/env tarantool
 
 local test = require('tap').test('log')
-test:plan(22)
+test:plan(23)
 
 --
 -- Check that Tarantool creates ADMIN session for #! script
@@ -102,5 +102,16 @@ test:ok(not line:match("{"), "log change format")
 s, e = pcall(log.log_format, "non_format")
 test:ok(not s)
 file:close()
+
+log.log_format("json")
+os.execute(string.format("mv %s %s2", filename, filename))
+log.rotate()
+log.info("some info")
+file = io.open(filename)
+line = file:read()
+message = json.decode(line)
+test:is(message.message, "log file has been reopened", "check message after log.rotate()")
+
+file:close()
 test:check()
 os.exit()
diff --git a/third_party/zstd b/third_party/zstd

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov



More information about the Tarantool-patches mailing list