Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reduce manual resource management #1046

Merged
merged 1 commit into from
Jan 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/glog/logging.h
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
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
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