From 94bab44c401ef0025aa670179ded946e73cc3d0c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 12:19:01 +0000 Subject: [PATCH] Address PR #799 review comments - windows/hid.c: fix typos in hid_is_read_interrupted comment. - linux/hid.c: skip close() in hid_close() when device_handle < 0 to avoid clobbering errno on the open-failure path. - linux/hid.c: preserve eventfd() errno across free() in new_hid_device(); report the real errno from hid_open_path() instead of overwriting with ENOMEM. - linux/hid.c, netbsd/hid.c: replace `volatile int interrupted` with __atomic_load_n/__atomic_store_n (acquire/release) so the cross-thread guarantee actually holds. The pipe/eventfd handles the wakeup; the atomic covers hid_is_read_interrupted and the early-out check. - hid_read_test/main.cpp: signal handler no longer calls hid_read_interrupt() (not async-signal-safe on libusb/mac). Handler now sets an atomic flag; main thread, woken by EINTR from cin.get(), calls hid_read_interrupt(). Use sigaction without SA_RESTART on POSIX. - hid_read_test/CMakeLists.txt: lower cmake_minimum_required to 3.1.3 to match the rest of the repo; switch from target_compile_features(cxx_std_11) (CMake 3.8+) to set_target_properties(CXX_STANDARD 11) (CMake 3.1+). https://claude.ai/code/session_01JB9WGTp5xipxBFMgcJa8XT --- hid_read_test/CMakeLists.txt | 20 ++++++++++++++++---- hid_read_test/main.cpp | 23 ++++++++++++++++++----- linux/hid.c | 18 ++++++++++-------- netbsd/hid.c | 10 +++++----- windows/hid.c | 4 ++-- 5 files changed, 51 insertions(+), 24 deletions(-) diff --git a/hid_read_test/CMakeLists.txt b/hid_read_test/CMakeLists.txt index 32e300f4..77f97669 100644 --- a/hid_read_test/CMakeLists.txt +++ b/hid_read_test/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.8...3.25 FATAL_ERROR) +cmake_minimum_required(VERSION 3.1.3...3.25 FATAL_ERROR) project(hid_read_test CXX) if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) @@ -19,20 +19,32 @@ set(HIDAPI_HID_READ_TEST_TARGETS) if(NOT WIN32 AND NOT APPLE AND CMAKE_SYSTEM_NAME MATCHES "Linux") if(TARGET hidapi::hidraw) add_executable(hid_read_test_hidraw main.cpp) - target_compile_features(hid_read_test_hidraw PRIVATE cxx_std_11) + set_target_properties(hid_read_test_hidraw PROPERTIES + CXX_STANDARD 11 + CXX_STANDARD_REQUIRED ON + CXX_EXTENSIONS OFF + ) target_link_libraries(hid_read_test_hidraw PRIVATE hidapi::hidraw Threads::Threads) list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test_hidraw) endif() if(TARGET hidapi::libusb) add_executable(hid_read_test_libusb main.cpp) - target_compile_features(hid_read_test_libusb PRIVATE cxx_std_11) + set_target_properties(hid_read_test_libusb PROPERTIES + CXX_STANDARD 11 + CXX_STANDARD_REQUIRED ON + CXX_EXTENSIONS OFF + ) target_compile_definitions(hid_read_test_libusb PRIVATE USING_HIDAPI_LIBUSB) target_link_libraries(hid_read_test_libusb PRIVATE hidapi::libusb Threads::Threads) list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test_libusb) endif() else() add_executable(hid_read_test main.cpp) - target_compile_features(hid_read_test PRIVATE cxx_std_11) + set_target_properties(hid_read_test PROPERTIES + CXX_STANDARD 11 + CXX_STANDARD_REQUIRED ON + CXX_EXTENSIONS OFF + ) target_link_libraries(hid_read_test PRIVATE hidapi::hidapi Threads::Threads) list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test) endif() diff --git a/hid_read_test/main.cpp b/hid_read_test/main.cpp index 454b5a3b..06db214f 100644 --- a/hid_read_test/main.cpp +++ b/hid_read_test/main.cpp @@ -24,9 +24,13 @@ #include #include +#ifndef _WIN32 +#include +#endif + namespace { -std::atomic g_dev{nullptr}; +std::atomic g_terminate{false}; std::string timestamp_now() { @@ -50,8 +54,9 @@ std::string timestamp_now() extern "C" void on_signal(int) { - if (hid_device *d = g_dev.load(std::memory_order_acquire)) - hid_read_interrupt(d); + /* async-signal-safe: atomic store only. Main thread will call + hid_read_interrupt() once cin.get() returns from EINTR. */ + g_terminate.store(true, std::memory_order_release); } void read_thread_fn(hid_device *dev) @@ -122,12 +127,20 @@ int main(int argc, char **argv) hid_exit(); return 1; } - g_dev.store(dev, std::memory_order_release); - +#ifdef _WIN32 std::signal(SIGINT, on_signal); #ifdef SIGTERM std::signal(SIGTERM, on_signal); #endif +#else + /* Use sigaction without SA_RESTART so cin.get() returns on signal. */ + struct sigaction sa; + sa.sa_handler = on_signal; + sigemptyset(&sa.sa_mask); + sa.sa_flags = 0; + sigaction(SIGINT, &sa, nullptr); + sigaction(SIGTERM, &sa, nullptr); +#endif std::cout << "Reading from VID=" << std::hex << std::setw(4) << std::setfill('0') << vid << " PID=" << std::setw(4) << pid << std::dec diff --git a/linux/hid.c b/linux/hid.c index e33c4b20..659f268c 100644 --- a/linux/hid.c +++ b/linux/hid.c @@ -80,7 +80,7 @@ struct hid_device_ { wchar_t *last_read_error_str; struct hid_device_info* device_info; int interrupt_efd; - volatile int interrupted; + int interrupted; }; static struct hid_api_version api_version = { @@ -107,7 +107,9 @@ static hid_device *new_hid_device(void) dev->interrupted = 0; dev->interrupt_efd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); if (dev->interrupt_efd < 0) { + int saved_errno = errno; free(dev); + errno = saved_errno; return NULL; } @@ -1091,8 +1093,7 @@ hid_device * HID_API_EXPORT hid_open_path(const char *path) dev = new_hid_device(); if (!dev) { - errno = ENOMEM; - register_global_error("Couldn't allocate memory"); + register_global_error_format("Failed to create hid_device: %s", strerror(errno)); return NULL; } @@ -1149,7 +1150,7 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t /* Set device error to none */ register_error_str(&dev->last_read_error_str, NULL); - if (dev->interrupted) { + if (__atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE)) { register_error_str(&dev->last_read_error_str, "hid_read_timeout: operation interrupted"); return -1; } @@ -1238,7 +1239,7 @@ int HID_API_EXPORT hid_set_nonblocking(hid_device *dev, int nonblock) int HID_API_EXPORT hid_read_interrupt(hid_device *dev) { - dev->interrupted = 1; + __atomic_store_n(&dev->interrupted, 1, __ATOMIC_RELEASE); uint64_t one = 1; ssize_t written = write(dev->interrupt_efd, &one, sizeof(one)); (void)written; @@ -1247,12 +1248,12 @@ int HID_API_EXPORT hid_read_interrupt(hid_device *dev) int HID_API_EXPORT hid_is_read_interrupted(hid_device *dev) { - return dev->interrupted; + return __atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE); } int HID_API_EXPORT hid_read_clear_interrupt(hid_device *dev) { - dev->interrupted = 0; + __atomic_store_n(&dev->interrupted, 0, __ATOMIC_RELEASE); uint64_t v; ssize_t got = read(dev->interrupt_efd, &v, sizeof(v)); (void)got; @@ -1341,7 +1342,8 @@ void HID_API_EXPORT hid_close(hid_device *dev) if (!dev) return; - close(dev->device_handle); + if (dev->device_handle >= 0) + close(dev->device_handle); if (dev->interrupt_efd >= 0) close(dev->interrupt_efd); diff --git a/netbsd/hid.c b/netbsd/hid.c index 801f07eb..383fcb11 100644 --- a/netbsd/hid.c +++ b/netbsd/hid.c @@ -60,7 +60,7 @@ struct hid_device_ { int report_handles[256]; char path[USB_MAX_DEVNAMELEN]; int interrupt_pipe[2]; - volatile int interrupted; + int interrupted; }; struct hid_enumerate_data { @@ -932,7 +932,7 @@ int HID_API_EXPORT HID_API_CALL hid_read_timeout(hid_device *dev, unsigned char register_device_read_error(dev, NULL); - if (dev->interrupted) { + if (__atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE)) { register_device_read_error(dev, "hid_read_timeout: operation interrupted"); return -1; } @@ -995,7 +995,7 @@ int HID_API_EXPORT HID_API_CALL hid_set_nonblocking(hid_device *dev, int nonbloc int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev) { - dev->interrupted = 1; + __atomic_store_n(&dev->interrupted, 1, __ATOMIC_RELEASE); const char one = 1; ssize_t written = write(dev->interrupt_pipe[1], &one, 1); (void)written; @@ -1004,12 +1004,12 @@ int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev) int HID_API_EXPORT HID_API_CALL hid_is_read_interrupted(hid_device *dev) { - return dev->interrupted; + return __atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE); } int HID_API_EXPORT HID_API_CALL hid_read_clear_interrupt(hid_device *dev) { - dev->interrupted = 0; + __atomic_store_n(&dev->interrupted, 0, __ATOMIC_RELEASE); char buf[64]; ssize_t got; do { diff --git a/windows/hid.c b/windows/hid.c index a11da24e..bbf196dd 100644 --- a/windows/hid.c +++ b/windows/hid.c @@ -1283,9 +1283,9 @@ int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev) int HID_API_EXPORT HID_API_CALL hid_is_read_interrupted(hid_device *dev) { - // TODO: very common but not the most efficient way to check this, + // TODO: common but not the most efficient way to check this; // move to C11 atomics when the time comes. - // For now this is the most portable and efficient enought. + // For now this is portable and efficient enough. return InterlockedCompareExchange(&dev->interrupted, 0, 0) ? 1 : 0; }