From 563e584da477fc9bff1936d537f2021addafaab3 Mon Sep 17 00:00:00 2001
From: Zygo Blaxell <bees@furryterror.org>
Date: Thu, 29 Dec 2022 04:02:41 -0500
Subject: [PATCH] task: use pthread_setname_np correctly

It turns out I've been using pthread_setname_np wrong the whole time:

 * on Linux, the thread name length is 15 characters.
   TASK_COMM_LEN is 16 bytes, and the last one is always 0.
   This is now hardcoded in many places and cannot be changed.

 * pthread_setname_np doesn't return -errno, so DIE_IF_MINUS_ERRNO
   was the wrong macro.  On the other hand, we never want to do anything
   differently when pthread_setname_np fails, so we never needed to
   check the return value.

Also, libc silently ignores attempts to set the thread name when it is too
long.  That's almost certainly a libc bug, but libc probably suppresses
the error result for the same reasons I ignore the error result.

Wrap the pthread_setname function with a C++ std::string overload that
truncates the argument at 15 characters, so we at least get the first
part of the task name in the thread name field.  Later commits can deal
with making the bees thread names shorter.

Also wrap pthread_getname for symmetry.

Signed-off-by: Zygo Blaxell <bees@furryterror.org>
---
 include/crucible/task.h |  4 ++++
 lib/task.cc             | 32 ++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/crucible/task.h b/include/crucible/task.h
index 7b7d132..8c9b321 100644
--- a/include/crucible/task.h
+++ b/include/crucible/task.h
@@ -174,7 +174,11 @@ namespace crucible {
 		void insert_task(const Task &t);
 	};
 
+	/// Wrapper around pthread_setname_np which handles length limits
+	void pthread_setname(const string &name);
 
+	/// Wrapper around pthread_getname_np for symmetry
+	string pthread_getname();
 }
 
 #endif // CRUCIBLE_TASK_H
diff --git a/lib/task.cc b/lib/task.cc
index 405cc97..9604ff2 100644
--- a/lib/task.cc
+++ b/lib/task.cc
@@ -18,6 +18,27 @@
 namespace crucible {
 	using namespace std;
 
+	static const size_t thread_name_length = 15; // TASK_COMM_LEN on Linux
+
+	void
+	pthread_setname(const string &name)
+	{
+		auto name_copy = name.substr(0, thread_name_length);
+		// Don't care if a debugging facility fails
+		pthread_setname_np(pthread_self(), name_copy.c_str());
+	}
+
+	string
+	pthread_getname()
+	{
+		char buf[thread_name_length + 1] = { 0 };
+		// We'll get an empty name if this fails...
+		pthread_getname_np(pthread_self(), buf, sizeof(buf));
+		// ...or at least null-terminated garbage
+		buf[thread_name_length] = '\0';
+		return buf;
+	}
+
 	class TaskState;
 	using TaskStatePtr = shared_ptr<TaskState>;
 	using TaskStateWeak = weak_ptr<TaskState>;
@@ -305,15 +326,14 @@ namespace crucible {
 		swap(this_task, tl_current_task);
 		lock.unlock();
 
-		char buf[64] = { 0 };
-		DIE_IF_MINUS_ERRNO(pthread_getname_np(pthread_self(), buf, sizeof(buf)));
-		DIE_IF_MINUS_ERRNO(pthread_setname_np(pthread_self(), m_title.c_str()));
+		const auto old_thread_name = pthread_getname();
+		pthread_setname(m_title);
 
 		catch_all([&]() {
 			m_exec_fn();
 		});
 
-		pthread_setname_np(pthread_self(), buf);
+		pthread_setname(old_thread_name);
 
 		lock.lock();
 		swap(this_task, tl_current_task);
@@ -608,7 +628,7 @@ namespace crucible {
 	void
 	TaskMasterState::loadavg_thread_fn()
 	{
-		pthread_setname_np(pthread_self(), "load_tracker");
+		pthread_setname("load_tracker");
 		while (!m_cancelled) {
 			adjust_thread_count();
 			nanosleep(5.0);
@@ -734,7 +754,7 @@ namespace crucible {
 		m_thread->detach();
 
 		// Set thread name so it isn't empty or the name of some other thread
-		DIE_IF_MINUS_ERRNO(pthread_setname_np(pthread_self(), "task_consumer"));
+		pthread_setname("task_consumer");
 
 		// It is now safe to access our own shared_ptr
 		TaskConsumerPtr this_consumer = shared_from_this();