Sherlock0203 commented on code in PR #3687:
URL: https://github.com/apache/brpc/pull/3687#discussion_r4511339914
##########
src/butil/logging.cc:
##########
@@ -425,6 +458,210 @@ void CloseLogFileUnlocked() {
CloseFile(log_file);
log_file = nullptr;
+ log_file_size = 0;
+}
+
+// ---------------------------- Log rotation ----------------------------
+// Rotation is size based and disabled by default. Once the current log file
+// is about to grow beyond --log_rotate_size_mb, it is renamed to
+// "<log_file_name>.1" and a fresh file is opened under the original name.
+// Existing backups are shifted down (".1" -> ".2" -> ...) beforehand and the
+// ones beyond --log_rotate_max_backups are removed, so that ".1" is always
+// the most recent backup. Every rotation also removes the backups left over
+// from a previously larger --log_rotate_max_backups, so that lowering the
+// flag at run time really lowers the number of files kept.
+//
+// A log file that disappeared from under the process is not a rotation
+// failure: the handle is closed so that the next write starts a fresh file,
+// and the backups are left untouched. The size at which it happened is
+// remembered all the same, so that a file which is still over the limit
+// once reopened (the ".\\debug.log" that Windows falls back to when the
+// requested name could not be opened at all) is not closed and reopened on
+// every single log.
+//
+// A rename that fails does not disable rotation for good, and a backup is
+// only ever replaced by a shift that succeeded: a backup that cannot be
+// moved out of the way aborts the rotation instead of being silently
+// overwritten with the current file. The size at which the rotation failed
+// is remembered and rotation is retried once the file has grown by another
+// --log_rotate_size_mb, which keeps a directory that cannot be written to
+// from closing, reopening and renaming the file for every single log. A
+// file that turns out to be smaller than it was at that point has been
+// replaced or truncated from the outside, and rotation resumes immediately.
+//
+// Sharing one log file between multiple processes is NOT supported together
+// with rotation: on POSIX LoggingLock only serializes the threads of a single
+// process, and after the rename the other processes would keep writing to the
+// renamed inode.
+
+// Return "<path>.<index>".
+PathString MakeBackupLogPath(const PathString& path, int index) {
+#if defined(OS_WIN)
+ wchar_t suffix[16];
+ swprintf(suffix, 16, L".%d", index);
+#else
+ char suffix[16];
+ snprintf(suffix, sizeof(suffix), ".%d", index);
+#endif
+ return path + suffix;
+}
+
+bool PathExists(const PathString& path) {
+#if defined(OS_WIN)
+ return GetFileAttributes(path.c_str()) != INVALID_FILE_ATTRIBUTES;
+#else
+ return ::access(path.c_str(), F_OK) == 0;
+#endif
+}
+
+bool RenameFilePath(const PathString& from, const PathString& to) {
+#if defined(OS_WIN)
+ return ::MoveFileEx(from.c_str(), to.c_str(),
+ MOVEFILE_REPLACE_EXISTING) != 0;
+#elif defined(OS_NACL)
+ // Do nothing; rename() isn't supported on NaCl.
+ return false;
+#else
+ return ::rename(from.c_str(), to.c_str()) == 0;
+#endif
+}
+
+// Narrow representation of a path, for printing. Paths are wide on Windows.
+std::string PathAsString(const PathString& path) {
+#if defined(OS_WIN)
+ return butil::WideToUTF8(path);
+#else
+ return path;
+#endif
+}
+
+// Back off after a rotation that could not rename `from' to `to': the size
+// that triggered it does not change by itself, so retrying on the next log
+// would close, reopen and rename the file for every single log. The failure
+// is printed once rather than per log. The caller must hold the logging
+// lock and `err' must have been captured before anything else could
+// overwrite it.
+void BackOffAfterFailedRotation(int64_t size_before_rotation,
+ const PathString& from,
+ const PathString& to,
+ SystemErrorCode err) {
+ failed_rotation_size = size_before_rotation;
+ if (rotation_failure_reported) {
+ return;
+ }
+ rotation_failure_reported = true;
+ fprintf(stderr, "Fail to rotate %s: renaming %s to %s failed: %s, "
+ "retrying after another --log_rotate_size_mb of logs\n",
+ PathAsString(*log_file_name).c_str(), PathAsString(from).c_str(),
+ PathAsString(to).c_str(),
+ SystemErrorCodeToString(err).c_str());
+}
+
+// Rename the current log file to "<log_file_name>.1" and shift the older
+// backups down by one. The caller must hold the logging lock.
+void RotateLogFileUnlocked() {
+ if (!log_file_name) {
+ return;
+ }
+ // Remember it before closing, CloseLogFileUnlocked() resets the size.
+ const int64_t size_before_rotation = log_file_size;
+ if (!PathExists(*log_file_name)) {
+ // The file was renamed or removed from the outside. There is nothing
+ // to rotate: closing the handle makes the next write start a fresh
+ // file under the original name, which is what a rotation would have
+ // done anyway. The backups are left alone, shifting them down would
+ // consume one generation for nothing. This is not an error, but the
+ // back off still applies: the file that the next write reopens may
+ // still be over the limit, which is the case of the Windows
+ // fallback to ".\\debug.log" when the requested name could not be
+ // opened at all. A fresh file is smaller than
+ // `size_before_rotation', which lets the next write retry at once.
+ CloseLogFileUnlocked();
+ failed_rotation_size = size_before_rotation;
+ return;
+ }
+ // Close before renaming: on Windows an opened file cannot be renamed, and
+ // on POSIX the handle would keep pointing at the renamed inode.
+ CloseLogFileUnlocked();
+
+ const int max_backups = std::max(1, FLAGS_log_rotate_max_backups);
+ // Remove the oldest backup, and any left over from a previously larger
+ // --log_rotate_max_backups, so that lowering the flag at run time really
+ // lowers the number of files kept.
+ for (int i = max_backups; ; ++i) {
+ const PathString path = MakeBackupLogPath(*log_file_name, i);
+ if (!PathExists(path)) {
+ break;
+ }
+ DeleteFilePath(path);
+ if (PathExists(path)) {
+ break; // could not remove it, do not spin
+ }
+ }
+ // Rotation numbers the backups contiguously, so the first missing index
+ // bounds the existing ones from above. Shifting from the highest
+ // existing index rather than from --log_rotate_max_backups keeps a huge
+ // value (only validated to be a positive int) from turning into as many
+ // rename() calls on every rotation, all under the logging lock.
+ int highest_backup = 0;
+ for (int i = 1; i < max_backups; ++i) {
+ if (!PathExists(MakeBackupLogPath(*log_file_name, i))) {
+ break;
+ }
+ highest_backup = i;
+ }
+ // Shift the backups down by one. A rename that fails aborts the
+ // rotation: renaming the current file over "<log_file_name>.1" anyway
+ // would destroy the newest backup, the very one that the failed shift
+ // could not move out of the way.
+ for (int i = highest_backup; i >= 1; --i) {
+ const PathString from = MakeBackupLogPath(*log_file_name, i);
+ const PathString to = MakeBackupLogPath(*log_file_name, i + 1);
+ if (!RenameFilePath(from, to)) {
+ BackOffAfterFailedRotation(size_before_rotation, from, to,
+ GetLastSystemErrorCode());
+ return;
+ }
+ }
+ const PathString first_backup = MakeBackupLogPath(*log_file_name, 1);
+ if (RenameFilePath(*log_file_name, first_backup)) {
+ failed_rotation_size = -1;
+ rotation_failure_reported = false;
+ return;
Review Comment:
Reproduced it the same way: 0600 file, one rotation, and the new file comes
back
with whatever the umask allows. Fixed in 55b18b30. The mode of the file being
replaced is taken with fstat() right before the rename — so a chmod() done
while the process is running is honored too, not just the mode the file was
opened with — and the fresh file is created with open(O_CREAT | O_EXCL, mode)
plus fchmod() to restore the exact bits. If the path already exists at that
point we just fopen() it and leave its permissions alone; a file somebody
else
created is never chmod'ed. rotation_keeps_the_file_mode covers both the file
and the rotated backup, including the chmod-while-running case. This is POSIX
only — on Windows the recreated file still gets whatever CreateFile gives a
new
file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
