This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 628c5d311 [minidump] a minor cleanup on the minidump code
628c5d311 is described below
commit 628c5d31198a8573db9c61b4a7beac0a090c79ae
Author: Alexey Serbin <[email protected]>
AuthorDate: Mon Jul 17 11:04:22 2023 -0700
[minidump] a minor cleanup on the minidump code
I looked through the code of the minidump wrapper in the context of
KUDU-3491 and cleaned up the related sections a bit:
* explicitly initialize the 'user_signal_handler_thread_running_' field
* changed the signature of the minidump_dir() method to return
'const std::string' and perform an extra sanity check (debug-only)
* changed the type from 'int' to 'size_t' and explicitly initialize
the 'current_num_instances_' static member: that's not necessary per
the C++ standard (at least C++17), but might help to work around
possible compiler bugs
* other minor updates: code style, etc.
Change-Id: I33b0c528975bf50a6fe16b9c2f76eeb685e5dd35
Reviewed-on: http://gerrit.cloudera.org:8080/20206
Reviewed-by: Abhishek Chennaka <[email protected]>
Tested-by: Alexey Serbin <[email protected]>
---
src/kudu/util/minidump.cc | 39 ++++++++++++++++++++++++---------------
src/kudu/util/minidump.h | 12 +++++++-----
2 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/src/kudu/util/minidump.cc b/src/kudu/util/minidump.cc
index 66310b4e6..e7daa403c 100644
--- a/src/kudu/util/minidump.cc
+++ b/src/kudu/util/minidump.cc
@@ -24,6 +24,7 @@
#include <csignal>
#include <cstdint>
#include <cstdlib>
+#include <ctime>
#include <functional>
#include <initializer_list>
#include <memory>
@@ -122,6 +123,8 @@ static sigset_t GetSigset(int signo) {
return signals;
}
+std::atomic<size_t> MinidumpExceptionHandler::current_num_instances_ = 0;
+
#if defined(__linux__)
// Called by the exception handler before minidump is produced.
@@ -204,7 +207,10 @@ bool MinidumpExceptionHandler::WriteMinidump() {
}
Status MinidumpExceptionHandler::InitMinidumpExceptionHandler() {
+ DCHECK(!breakpad_handler_);
+
minidump_dir_ = FLAGS_minidump_path;
+ CHECK(!minidump_dir_.empty());
if (minidump_dir_[0] != '/') {
minidump_dir_ = JoinPathSegments(FLAGS_log_dir, minidump_dir_);
}
@@ -267,20 +273,24 @@ Status
MinidumpExceptionHandler::InitMinidumpExceptionHandler() {
}
Status MinidumpExceptionHandler::RegisterMinidumpExceptionHandler() {
- if (!FLAGS_enable_minidumps) return Status::OK();
+ if (!FLAGS_enable_minidumps) {
+ return Status::OK();
+ }
// Ensure only one active instance is alive per process at any given time.
- CHECK_EQ(0, MinidumpExceptionHandler::current_num_instances_.fetch_add(1));
+ CHECK_EQ(0, current_num_instances_.fetch_add(1));
RETURN_NOT_OK(InitMinidumpExceptionHandler());
- RETURN_NOT_OK(StartUserSignalHandlerThread());
- return Status::OK();
+
+ return StartUserSignalHandlerThread();
}
void MinidumpExceptionHandler::UnregisterMinidumpExceptionHandler() {
- if (!FLAGS_enable_minidumps) return;
+ if (!FLAGS_enable_minidumps) {
+ return;
+ }
StopUserSignalHandlerThread();
- CHECK_EQ(1, MinidumpExceptionHandler::current_num_instances_.fetch_sub(1));
+ CHECK_EQ(1, current_num_instances_.fetch_sub(1));
}
Status MinidumpExceptionHandler::StartUserSignalHandlerThread() {
@@ -315,16 +325,16 @@ void
MinidumpExceptionHandler::RunUserSignalHandlerThread() {
}
}
-#else // defined(__linux__)
+#else // #if defined(__linux__) ...
-// At the time of writing, we don't support breakpad on Mac so we just stub out
-// all the methods defined in the header file.
+// At the time of writing, we support breakpad only on Linux, so we just stub
+// out all the methods defined in the header file, i.e. the methods implemented
+// below in the '#else' macro's scope are all no-op.
Status MinidumpExceptionHandler::InitMinidumpExceptionHandler() {
return Status::OK();
}
-// No-op on non-Linux platforms.
Status MinidumpExceptionHandler::RegisterMinidumpExceptionHandler() {
return Status::OK();
}
@@ -346,9 +356,7 @@ void
MinidumpExceptionHandler::StopUserSignalHandlerThread() {
void MinidumpExceptionHandler::RunUserSignalHandlerThread() {
}
-#endif // defined(__linux__)
-
-std::atomic<int> MinidumpExceptionHandler::current_num_instances_;
+#endif // if defined(__linux__) ... #else ...
MinidumpExceptionHandler::MinidumpExceptionHandler() {
CHECK_OK(RegisterMinidumpExceptionHandler());
@@ -362,7 +370,7 @@ Status
MinidumpExceptionHandler::DeleteExcessMinidumpFiles(Env* env) {
// Do not delete minidump files if minidumps are disabled.
if (!FLAGS_enable_minidumps) return Status::OK();
- int32_t max_minidumps = FLAGS_max_minidumps;
+ const int32_t max_minidumps = FLAGS_max_minidumps;
// Disable rotation if set to 0 or less.
if (max_minidumps <= 0) return Status::OK();
@@ -377,7 +385,8 @@ Status
MinidumpExceptionHandler::DeleteExcessMinidumpFiles(Env* env) {
return env_util::DeleteExcessFilesByPattern(env, pattern, max_minidumps);
}
-string MinidumpExceptionHandler::minidump_dir() const {
+const string& MinidumpExceptionHandler::minidump_dir() const {
+ DCHECK(breakpad_handler_);
return minidump_dir_;
}
diff --git a/src/kudu/util/minidump.h b/src/kudu/util/minidump.h
index 893440ae6..beaaa3478 100644
--- a/src/kudu/util/minidump.h
+++ b/src/kudu/util/minidump.h
@@ -18,6 +18,7 @@
#pragma once
#include <atomic>
+#include <cstddef>
#include <memory>
#include <string>
@@ -65,7 +66,7 @@ class MinidumpExceptionHandler {
Status DeleteExcessMinidumpFiles(Env* env);
// Get the path to the directory that will be used for writing minidumps.
- std::string minidump_dir() const;
+ const std::string& minidump_dir() const;
private:
Status InitMinidumpExceptionHandler();
@@ -79,11 +80,12 @@ class MinidumpExceptionHandler {
// The number of instnaces of this class that are currently in existence.
// We keep this counter in order to force a crash if more than one is running
// at a time, as a sanity check.
- static std::atomic<int> current_num_instances_;
+ static std::atomic<size_t> current_num_instances_;
- #ifndef __APPLE__
- std::atomic<bool> user_signal_handler_thread_running_;// Unused in macOS
build.
- #endif
+#ifndef __APPLE__
+ // Unused in macOS build.
+ std::atomic<bool> user_signal_handler_thread_running_ = false;
+#endif
scoped_refptr<Thread> user_signal_handler_thread_;
