Skip to content

Commit f786335

Browse files
committed
Address review: helper consistency, overridable cap, ring buffer
Per-backend helper consistency: * linux/hid.c, netbsd/hid.c, mac/hid.c: setter uses register_device_error() instead of register_error_str() directly. Build-time override: * hidapi/hidapi.h: wrap HID_API_MAX_NUM_INPUT_BUFFERS in #ifndef so downstreams can set the cap via -DHID_API_MAX_NUM_INPUT_BUFFERS=<value>. Ring buffer input queue: * New static-in-header helper hidapi_input_ring_*, present in libusb/ and mac/ as byte-identical copies. * libusb/hid.c, mac/hid.c: replace struct input_report * linked list with fixed-size ring. Enqueue is O(1); eviction is inline in push; the setter shrinks via drop_oldest so dev->num_input_buffers is the exact steady-state cap. * ABI unchanged (hid_device is opaque in hidapi.h). * Allocation failure in the read callback is now handled — the previous code had an unchecked malloc() that would segfault. libusb has no active error channel so the drop is silent there; mac calls register_device_error.
1 parent 4364aed commit f786335

7 files changed

Lines changed: 520 additions & 143 deletions

File tree

hidapi/hidapi.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,8 +428,13 @@ extern "C" {
428428
Values passed above this limit are rejected by
429429
hid_set_num_input_buffers(). Guards against
430430
memory-exhaustion via unbounded input report queue growth.
431+
432+
May be overridden at build time via
433+
-DHID_API_MAX_NUM_INPUT_BUFFERS=<value>.
431434
*/
435+
#ifndef HID_API_MAX_NUM_INPUT_BUFFERS
432436
#define HID_API_MAX_NUM_INPUT_BUFFERS 1024
437+
#endif
433438

434439
/** @brief Set the number of input report buffers queued per device.
435440

libusb/hid.c

Lines changed: 45 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
#endif
5050

5151
#include "hidapi_libusb.h"
52+
#include "hidapi_input_ring.h"
5253

5354
#ifndef HIDAPI_THREAD_MODEL_INCLUDE
5455
#define HIDAPI_THREAD_MODEL_INCLUDE "hidapi_thread_pthread.h"
@@ -77,13 +78,6 @@ libusb HIDAPI programs are encouraged to use the interface number
7778
instead to differentiate between interfaces on a composite HID device. */
7879
/*#define INVASIVE_GET_USAGE*/
7980

80-
/* Linked List of input reports received from the device. */
81-
struct input_report {
82-
uint8_t *data;
83-
size_t len;
84-
struct input_report *next;
85-
};
86-
8781

8882
struct hid_device_ {
8983
/* Handle to the actual device. */
@@ -119,8 +113,10 @@ struct hid_device_ {
119113
int transfer_loop_finished;
120114
struct libusb_transfer *transfer;
121115

122-
/* List of received input reports. */
123-
struct input_report *input_reports;
116+
/* Input report ring buffer. Backing array sized at
117+
HID_API_MAX_NUM_INPUT_BUFFERS at device open; logical cap
118+
(drop-oldest threshold) is dev->num_input_buffers. */
119+
struct hidapi_input_ring input_ring;
124120

125121
/* Was kernel driver detached by libusb */
126122
#ifdef DETACH_KERNEL_DRIVER
@@ -137,14 +133,18 @@ static struct hid_api_version api_version = {
137133
static libusb_context *usb_context = NULL;
138134

139135
uint16_t get_usb_code_for_current_locale(void);
140-
static int return_data(hid_device *dev, unsigned char *data, size_t length);
141136

142137
static hid_device *new_hid_device(void)
143138
{
144139
hid_device *dev = (hid_device*) calloc(1, sizeof(hid_device));
145140
if (!dev)
146141
return NULL;
147142

143+
if (hidapi_input_ring_init(&dev->input_ring, HID_API_MAX_NUM_INPUT_BUFFERS) != 0) {
144+
free(dev);
145+
return NULL;
146+
}
147+
148148
dev->blocking = 1;
149149
dev->num_input_buffers = 30;
150150

@@ -160,6 +160,9 @@ static void free_hid_device(hid_device *dev)
160160

161161
hid_free_enumeration(dev->device_info);
162162

163+
/* Free any queued input reports and the ring backing array. */
164+
hidapi_input_ring_destroy(&dev->input_ring);
165+
163166
/* Free the device itself */
164167
free(dev);
165168
}
@@ -962,37 +965,18 @@ static void LIBUSB_CALL read_callback(struct libusb_transfer *transfer)
962965

963966
if (transfer->status == LIBUSB_TRANSFER_COMPLETED) {
964967

965-
struct input_report *rpt = (struct input_report*) malloc(sizeof(*rpt));
966-
rpt->data = (uint8_t*) malloc(transfer->actual_length);
967-
memcpy(rpt->data, transfer->buffer, transfer->actual_length);
968-
rpt->len = transfer->actual_length;
969-
rpt->next = NULL;
970-
971968
hidapi_thread_mutex_lock(&dev->thread_state);
972969

973-
/* Attach the new report object to the end of the list. */
974-
if (dev->input_reports == NULL) {
975-
/* The list is empty. Put it at the root. */
976-
dev->input_reports = rpt;
970+
int push_rc = hidapi_input_ring_push(&dev->input_ring,
971+
dev->num_input_buffers,
972+
transfer->buffer,
973+
(size_t)transfer->actual_length);
974+
if (push_rc == 0) {
977975
hidapi_thread_cond_signal(&dev->thread_state);
978976
}
979-
else {
980-
/* Find the end of the list and attach. */
981-
struct input_report *cur = dev->input_reports;
982-
int num_queued = 0;
983-
while (cur->next != NULL) {
984-
cur = cur->next;
985-
num_queued++;
986-
}
987-
cur->next = rpt;
977+
/* Allocation failed; libusb has no active error channel here, so the
978+
* report is dropped silently. */
988979

989-
/* Pop one off if we've reached 30 in the queue. This
990-
way we don't grow forever if the user never reads
991-
anything from the device. */
992-
if (num_queued > dev->num_input_buffers) {
993-
return_data(dev, NULL, 0);
994-
}
995-
}
996980
hidapi_thread_mutex_unlock(&dev->thread_state);
997981
}
998982
else if (transfer->status == LIBUSB_TRANSFER_CANCELLED) {
@@ -1458,19 +1442,18 @@ int HID_API_EXPORT hid_write(hid_device *dev, const unsigned char *data, size_t
14581442
return actual_length;
14591443
}
14601444

1461-
/* Helper function, to simplify hid_read().
1462-
This should be called with dev->mutex locked. */
1463-
static int return_data(hid_device *dev, unsigned char *data, size_t length)
1445+
/* Pop one report from the ring into (data, length). Caller must
1446+
hold dev->thread_state. Returns bytes copied, or -1 if empty. */
1447+
static int ring_pop_into(hid_device *dev, unsigned char *data, size_t length)
14641448
{
1465-
/* Copy the data out of the linked list item (rpt) into the
1466-
return buffer (data), and delete the liked list item. */
1467-
struct input_report *rpt = dev->input_reports;
1468-
size_t len = (length < rpt->len)? length: rpt->len;
1469-
if (len > 0)
1470-
memcpy(data, rpt->data, len);
1471-
dev->input_reports = rpt->next;
1472-
free(rpt->data);
1473-
free(rpt);
1449+
uint8_t *rpt_data;
1450+
size_t rpt_len;
1451+
if (hidapi_input_ring_pop(&dev->input_ring, &rpt_data, &rpt_len) != 0)
1452+
return -1;
1453+
size_t len = (length < rpt_len) ? length : rpt_len;
1454+
if (len > 0 && data)
1455+
memcpy(data, rpt_data, len);
1456+
free(rpt_data);
14741457
return (int)len;
14751458
}
14761459

@@ -1499,9 +1482,8 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t
14991482
bytes_read = -1;
15001483

15011484
/* There's an input report queued up. Return it. */
1502-
if (dev->input_reports) {
1503-
/* Return the first one */
1504-
bytes_read = return_data(dev, data, length);
1485+
if (dev->input_ring.count > 0) {
1486+
bytes_read = ring_pop_into(dev, data, length);
15051487
goto ret;
15061488
}
15071489

@@ -1514,11 +1496,11 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t
15141496

15151497
if (milliseconds == -1) {
15161498
/* Blocking */
1517-
while (!dev->input_reports && !dev->shutdown_thread) {
1499+
while (dev->input_ring.count == 0 && !dev->shutdown_thread) {
15181500
hidapi_thread_cond_wait(&dev->thread_state);
15191501
}
1520-
if (dev->input_reports) {
1521-
bytes_read = return_data(dev, data, length);
1502+
if (dev->input_ring.count > 0) {
1503+
bytes_read = ring_pop_into(dev, data, length);
15221504
}
15231505
}
15241506
else if (milliseconds > 0) {
@@ -1528,11 +1510,11 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t
15281510
hidapi_thread_gettime(&ts);
15291511
hidapi_thread_addtime(&ts, milliseconds);
15301512

1531-
while (!dev->input_reports && !dev->shutdown_thread) {
1513+
while (dev->input_ring.count == 0 && !dev->shutdown_thread) {
15321514
res = hidapi_thread_cond_timedwait(&dev->thread_state, &ts);
15331515
if (res == 0) {
1534-
if (dev->input_reports) {
1535-
bytes_read = return_data(dev, data, length);
1516+
if (dev->input_ring.count > 0) {
1517+
bytes_read = ring_pop_into(dev, data, length);
15361518
break;
15371519
}
15381520

@@ -1585,6 +1567,10 @@ int HID_API_EXPORT hid_set_num_input_buffers(hid_device *dev, int num_buffers)
15851567
return -1;
15861568
hidapi_thread_mutex_lock(&dev->thread_state);
15871569
dev->num_input_buffers = num_buffers;
1570+
if (dev->input_ring.count > num_buffers) {
1571+
hidapi_input_ring_drop_oldest(&dev->input_ring,
1572+
dev->input_ring.count - num_buffers);
1573+
}
15881574
hidapi_thread_mutex_unlock(&dev->thread_state);
15891575
return 0;
15901576
}
@@ -1749,12 +1735,8 @@ void HID_API_EXPORT hid_close(hid_device *dev)
17491735
/* Close the handle */
17501736
libusb_close(dev->device_handle);
17511737

1752-
/* Clear out the queue of received reports. */
1753-
hidapi_thread_mutex_lock(&dev->thread_state);
1754-
while (dev->input_reports) {
1755-
return_data(dev, NULL, 0);
1756-
}
1757-
hidapi_thread_mutex_unlock(&dev->thread_state);
1738+
/* Queued reports are freed inside free_hid_device via
1739+
hidapi_input_ring_destroy. */
17581740

17591741
free_hid_device(dev);
17601742
}

0 commit comments

Comments
 (0)