Skip to content

Commit

Permalink
fix: reduce manual resource management
Browse files Browse the repository at this point in the history
  • Loading branch information
@sergiud
sergiud committed Jan 7, 2024
1 parent 1f9ac49 commit 81e49c9
Show file tree
Hide file tree
Showing 10 changed files with 237 additions and 187 deletions.
9 changes: 5 additions & 4 deletions src/glog/logging.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -965,12 +965,13 @@ namespace google {
LOG_OCCURRENCES, &what_to_do) \
.stream()

namespace glog_internal_namespace_ {
namespace logging {
namespace internal {
template <bool>
struct CompileAssert {};
struct CrashReason;

} // namespace glog_internal_namespace_
} // namespace internal
} // namespace logging

#define LOG_EVERY_N(severity, n) \
SOME_KIND_OF_LOG_EVERY_N(severity, (n), google::LogMessage::SendToLog)
Expand Down Expand Up @@ -1334,7 +1335,7 @@ class GLOG_EXPORT LogMessage {
void (LogMessage::*send_method)());

// Used to fill in crash information during LOG(FATAL) failures.
void RecordCrashReason(glog_internal_namespace_::CrashReason* reason);
void RecordCrashReason(logging::internal::CrashReason* reason);

// Counts of messages sent at each priority:
static int64 num_messages_[NUM_SEVERITIES]; // under log_mutex
Expand Down
83 changes: 35 additions & 48 deletions src/googletest.h
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2009, Google Inc.
// Copyright (c) 2024, Google Inc.
// All rights reserved.
//
// Redistribution and use in source and binary forms, with or without
Expand Down Expand Up @@ -73,7 +73,7 @@ extern void (*g_logging_fail_func)();
extern void GetExistingTempDirectories(std::vector<std::string>& list);
extern int posix_strerror_r(int err, char* buf, size_t len);
extern std::string StrError(int err);
}
} // namespace google

#undef GLOG_EXPORT
#define GLOG_EXPORT
Expand Down Expand Up @@ -300,60 +300,53 @@ static inline void RunSpecifiedBenchmarks() {
class CapturedStream {
public:
CapturedStream(int fd, string filename)
: fd_(fd),

filename_(std::move(filename)) {
: fd_(fd), filename_(std::move(filename)) {
Capture();
}

~CapturedStream() {
if (uncaptured_fd_ != -1) {
CHECK(close(uncaptured_fd_) != -1);
}
}

// Start redirecting output to a file
void Capture() {
// Keep original stream for later
CHECK(uncaptured_fd_ == -1) << ", Stream " << fd_ << " already captured!";
uncaptured_fd_ = dup(fd_);
CHECK(uncaptured_fd_ != -1);
CHECK(!uncaptured_fd_) << ", Stream " << fd_ << " already captured!";
uncaptured_fd_.reset(dup(fd_));
CHECK(uncaptured_fd_);

// Open file to save stream to
int cap_fd = open(filename_.c_str(), O_CREAT | O_TRUNC | O_WRONLY,
S_IRUSR | S_IWUSR);
CHECK(cap_fd != -1);
FileDescriptor cap_fd{open(filename_.c_str(), O_CREAT | O_TRUNC | O_WRONLY,
S_IRUSR | S_IWUSR)};
CHECK(cap_fd);

// Send stdout/stderr to this file
fflush(nullptr);
CHECK(dup2(cap_fd, fd_) != -1);
CHECK(close(cap_fd) != -1);
CHECK(dup2(cap_fd.get(), fd_) != -1);
CHECK(cap_fd.close() != -1);
}

// Remove output redirection
void StopCapture() {
// Restore original stream
if (uncaptured_fd_ != -1) {
if (uncaptured_fd_) {
fflush(nullptr);
CHECK(dup2(uncaptured_fd_, fd_) != -1);
CHECK(dup2(uncaptured_fd_.get(), fd_) != -1);
}
}

const string& filename() const { return filename_; }

private:
int fd_; // file descriptor being captured
int uncaptured_fd_{-1}; // where the stream was originally being sent to
string filename_; // file where stream is being saved
int fd_; // file descriptor being captured
FileDescriptor
uncaptured_fd_; // where the stream was originally being sent to
string filename_; // file where stream is being saved
};
static CapturedStream* s_captured_streams[STDERR_FILENO + 1];
static std::unique_ptr<CapturedStream> s_captured_streams[STDERR_FILENO + 1];
// Redirect a file descriptor to a file.
// fd - Should be STDOUT_FILENO or STDERR_FILENO
// filename - File where output should be stored
static inline void CaptureTestOutput(int fd, const string& filename) {
CHECK((fd == STDOUT_FILENO) || (fd == STDERR_FILENO));
CHECK(s_captured_streams[fd] == nullptr);
s_captured_streams[fd] = new CapturedStream(fd, filename);
s_captured_streams[fd] = std::make_unique<CapturedStream>(fd, filename);
}
static inline void CaptureTestStdout() {
CaptureTestOutput(STDOUT_FILENO, FLAGS_test_tmpdir + "/captured.out");
Expand All @@ -369,7 +362,7 @@ static inline size_t GetFileSize(FILE* file) {
// Read the entire content of a file as a string
static inline string ReadEntireFile(FILE* file) {
const size_t file_size = GetFileSize(file);
char* const buffer = new char[file_size];
std::vector<char> content(file_size);

size_t bytes_last_read = 0; // # of bytes read in the last fread()
size_t bytes_read = 0; // # of bytes read so far
Expand All @@ -380,32 +373,26 @@ static inline string ReadEntireFile(FILE* file) {
// pre-determined file size is reached.
do {
bytes_last_read =
fread(buffer + bytes_read, 1, file_size - bytes_read, file);
fread(content.data() + bytes_read, 1, file_size - bytes_read, file);
bytes_read += bytes_last_read;
} while (bytes_last_read > 0 && bytes_read < file_size);

const string content = string(buffer, buffer + bytes_read);
delete[] buffer;

return content;
return std::string(content.data(), bytes_read);
}
// Get the captured stdout (when fd is STDOUT_FILENO) or stderr (when
// fd is STDERR_FILENO) as a string
static inline string GetCapturedTestOutput(int fd) {
CHECK(fd == STDOUT_FILENO || fd == STDERR_FILENO);
CapturedStream* const cap = s_captured_streams[fd];
std::unique_ptr<CapturedStream> cap = std::move(s_captured_streams[fd]);
CHECK(cap) << ": did you forget CaptureTestStdout() or CaptureTestStderr()?";

// Make sure everything is flushed.
cap->StopCapture();

// Read the captured file.
FILE* const file = fopen(cap->filename().c_str(), "r");
const string content = ReadEntireFile(file);
fclose(file);

delete cap;
s_captured_streams[fd] = nullptr;
std::unique_ptr<FILE> file{fopen(cap->filename().c_str(), "r")};
const string content = ReadEntireFile(file.get());
file.reset();

return content;
}
Expand Down Expand Up @@ -479,11 +466,11 @@ static inline void StringReplace(string* str, const string& oldsub,
}

static inline string Munge(const string& filename) {
FILE* fp = fopen(filename.c_str(), "rb");
std::unique_ptr<FILE> fp{fopen(filename.c_str(), "rb")};
CHECK(fp != nullptr) << filename << ": couldn't open";
char buf[4096];
string result;
while (fgets(buf, 4095, fp)) {
while (fgets(buf, 4095, fp.get())) {
string line = MungeLine(buf);
const size_t str_size = 256;
char null_str[str_size];
Expand All @@ -502,19 +489,17 @@ static inline string Munge(const string& filename) {
StringReplace(&line, "__ENOEXEC__", StrError(ENOEXEC));
result += line + "\n";
}
fclose(fp);
return result;
}

static inline void WriteToFile(const string& body, const string& file) {
FILE* fp = fopen(file.c_str(), "wb");
fwrite(body.data(), 1, body.size(), fp);
fclose(fp);
std::unique_ptr<FILE> fp{fopen(file.c_str(), "wb")};
fwrite(body.data(), 1, body.size(), fp.get());
}

static inline bool MungeAndDiffTest(const string& golden_filename,
CapturedStream* cap) {
if (cap == s_captured_streams[STDOUT_FILENO]) {
if (cap == s_captured_streams[STDOUT_FILENO].get()) {
CHECK(cap) << ": did you forget CaptureTestStdout()?";
} else {
CHECK(cap) << ": did you forget CaptureTestStderr()?";
Expand Down Expand Up @@ -549,11 +534,13 @@ static inline bool MungeAndDiffTest(const string& golden_filename,
}

static inline bool MungeAndDiffTestStderr(const string& golden_filename) {
return MungeAndDiffTest(golden_filename, s_captured_streams[STDERR_FILENO]);
return MungeAndDiffTest(golden_filename,
s_captured_streams[STDERR_FILENO].get());
}

static inline bool MungeAndDiffTestStdout(const string& golden_filename) {
return MungeAndDiffTest(golden_filename, s_captured_streams[STDOUT_FILENO]);
return MungeAndDiffTest(golden_filename,
s_captured_streams[STDOUT_FILENO].get());
}

// Save flags used from logging_unittest.cc.
Expand Down
48 changes: 23 additions & 25 deletions src/logging.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1011,11 +1011,12 @@ bool LogFileObject::CreateLogfile(const string& time_pid_string) {
// demand that the file is unique for our timestamp (fail if it exists).
flags = flags | O_EXCL;
}
int fd = open(filename, flags, static_cast<mode_t>(FLAGS_logfile_mode));
if (fd == -1) return false;
FileDescriptor fd{
open(filename, flags, static_cast<mode_t>(FLAGS_logfile_mode))};
if (!fd) return false;
#ifdef HAVE_FCNTL
// Mark the file close-on-exec. We don't really care if this fails
fcntl(fd, F_SETFD, FD_CLOEXEC);
fcntl(fd.get(), F_SETFD, FD_CLOEXEC);

// Mark the file as exclusive write access to avoid two clients logging to the
// same file. This applies particularly when !FLAGS_timestamp_in_logfile_name
Expand All @@ -1034,17 +1035,15 @@ bool LogFileObject::CreateLogfile(const string& time_pid_string) {
w_lock.l_whence = SEEK_SET;
w_lock.l_len = 0;

int wlock_ret = fcntl(fd, F_SETLK, &w_lock);
int wlock_ret = fcntl(fd.get(), F_SETLK, &w_lock);
if (wlock_ret == -1) {
close(fd); // as we are failing already, do not check errors here
return false;
}
#endif

// fdopen in append mode so if the file exists it will fseek to the end
file_.reset(fdopen(fd, "a")); // Make a FILE*.
if (file_ == nullptr) { // Man, we're screwed!
close(fd);
file_.reset(fdopen(fd.release(), "a")); // Make a FILE*.
if (file_ == nullptr) { // Man, we're screwed!
if (FLAGS_timestamp_in_logfile_name) {
unlink(filename); // Erase the half-baked evidence: an unusable log file,
// only if we just created it.
Expand Down Expand Up @@ -1506,7 +1505,7 @@ bool LogCleaner::IsLogLastModifiedOver(
// for exclusive use by the first thread, and one for shared use by
// all other threads.
static std::mutex fatal_msg_lock;
static CrashReason crash_reason;
static logging::internal::CrashReason crash_reason;
static bool fatal_msg_exclusive = true;
static LogMessage::LogMessageData fatal_msg_data_exclusive;
static LogMessage::LogMessageData fatal_msg_data_shared;
Expand Down Expand Up @@ -1861,8 +1860,7 @@ void LogMessage::SendToLog() EXCLUSIVE_LOCKS_REQUIRED(log_mutex) {
}
}

void LogMessage::RecordCrashReason(
glog_internal_namespace_::CrashReason* reason) {
void LogMessage::RecordCrashReason(logging::internal::CrashReason* reason) {
reason->filename = fatal_msg_data_exclusive.fullname_;
reason->line_number = fatal_msg_data_exclusive.line_;
reason->message = fatal_msg_data_exclusive.message_text_ +
Expand Down Expand Up @@ -2396,16 +2394,16 @@ void TruncateLogFile(const char* path, uint64 limit, uint64 keep) {
if (strncmp(procfd_prefix, path, strlen(procfd_prefix))) flags |= O_NOFOLLOW;
# endif

int fd = open(path, flags);
if (fd == -1) {
FileDescriptor fd{open(path, flags)};
if (!fd) {
if (errno == EFBIG) {
// The log file in question has got too big for us to open. The
// real fix for this would be to compile logging.cc (or probably
// all of base/...) with -D_FILE_OFFSET_BITS=64 but that's
// rather scary.
// Instead just truncate the file to something we can manage
# ifdef HAVE__CHSIZE_S
if (_chsize_s(fd, 0) != 0) {
if (_chsize_s(fd.get(), 0) != 0) {
# else
if (truncate(path, 0) == -1) {
# endif
Expand All @@ -2419,16 +2417,16 @@ void TruncateLogFile(const char* path, uint64 limit, uint64 keep) {
return;
}

if (fstat(fd, &statbuf) == -1) {
if (fstat(fd.get(), &statbuf) == -1) {
PLOG(ERROR) << "Unable to fstat()";
goto out_close_fd;
return;
}

// See if the path refers to a regular file bigger than the
// specified limit
if (!S_ISREG(statbuf.st_mode)) goto out_close_fd;
if (statbuf.st_size <= static_cast<off_t>(limit)) goto out_close_fd;
if (statbuf.st_size <= static_cast<off_t>(keep)) goto out_close_fd;
if (!S_ISREG(statbuf.st_mode)) return;
if (statbuf.st_size <= static_cast<off_t>(limit)) return;
if (statbuf.st_size <= static_cast<off_t>(keep)) return;

// This log file is too large - we need to truncate it
LOG(INFO) << "Truncating " << path << " to " << keep << " bytes";
Expand All @@ -2437,8 +2435,10 @@ void TruncateLogFile(const char* path, uint64 limit, uint64 keep) {
read_offset = statbuf.st_size - static_cast<off_t>(keep);
write_offset = 0;
ssize_t bytesin, bytesout;
while ((bytesin = pread(fd, copybuf, sizeof(copybuf), read_offset)) > 0) {
bytesout = pwrite(fd, copybuf, static_cast<size_t>(bytesin), write_offset);
while ((bytesin = pread(fd.get(), copybuf, sizeof(copybuf), read_offset)) >
0) {
bytesout =
pwrite(fd.get(), copybuf, static_cast<size_t>(bytesin), write_offset);
if (bytesout == -1) {
PLOG(ERROR) << "Unable to write to " << path;
break;
Expand All @@ -2454,15 +2454,13 @@ void TruncateLogFile(const char* path, uint64 limit, uint64 keep) {
// end of the file after our last read() above, we lose their latest
// data. Too bad ...
# ifdef HAVE__CHSIZE_S
if (_chsize_s(fd, write_offset) != 0) {
if (_chsize_s(fd.get(), write_offset) != 0) {
# else
if (ftruncate(fd, write_offset) == -1) {
if (ftruncate(fd.get(), write_offset) == -1) {
# endif
PLOG(ERROR) << "Unable to truncate " << path;
}

out_close_fd:
close(fd);
#else
LOG(ERROR) << "No log truncation support.";
#endif
Expand Down
Loading

0 comments on commit 81e49c9

Please sign in to comment.