Skip to content

tests: hotplug API and virtual-device hotplug scenarios - #826

Open
Youw wants to merge 4 commits into
connection-callbackfrom
hotplug-tests
Open

tests: hotplug API and virtual-device hotplug scenarios#826
Youw wants to merge 4 commits into
connection-callbackfrom
hotplug-tests

Conversation

@Youw

@Youw Youw commented Jul 14, 2026

Copy link
Copy Markdown
Member

Adds the test suite for the hotplug API (#764 / connection-callback), in two tiers, built on the virtual-device test harness from #815.

What is in here

  • src/tests/test_hotplug_api.c — tier 1, HotplugAPI_<backend>: everything in the hotplug contract that is observable without a device event (argument validation, callback-handle properties, implicit hid_init(), hid_exit() teardown incl. a register→immediate-hid_exit stress loop, and a 2-thread register/deregister churn). Needs no device, no privileges: registered for all four backends and runs in the ordinary per-push CI matrix. Self-skips (77) when a backend reports hotplug as unsupported at runtime (e.g. libusb without LIBUSB_CAP_HAS_HOTPLUG).
  • src/tests/test_hotplug.c — tier 2, Hotplug_<backend>: device-backed scenarios: async delivery, exactly-once ENUMERATE pass, callback-return deregistration (live + mid-pass), pass-before-live ordering, ARRIVED/LEFT payloads and path correlation, LEFT-without-ENUMERATE, VID/PID filtering, registration-order dispatch, the deregistration post-condition, and register/deregister from within a callback.
  • test_virtual_device_unplug() / test_virtual_device_replug() — new provider calls that toggle device presence without destroying the provider context. Implemented for the uhid provider (UHID_DESTROY/UHID_CREATE2 on the same fd); the rawgadget/vhidmini2/IOHIDUserDevice providers return TEST_VDEV_UNAVAILABLE for now, so their Hotplug_* tests self-skip everywhere (they compile and are registered, ready for when toggling is implemented).

The tier-2 test follows a strict synchronization discipline (see src/tests/README.md): callbacks only deep-copy events into a lock-protected log; every expectation is a deadline-based predicate poll; absence assertions sit behind event barriers, not time windows; no retries.

Where each test runs

Test Where it runs
HotplugAPI_hidraw, HotplugAPI_libusb per-push, builds.yml ubuntu-cmake
HotplugAPI_winapi per-push, builds.yml windows-cmake (MSVC, NMake, ClangCL, MinGW)
HotplugAPI_darwin per-push, builds.yml macos-cmake
Hotplug_hidraw per-push, builds.yml ubuntu-cmake (via modprobe uhid + sudo ctest)
Hotplug_libusb builds + self-skips (incl. in the dedicated libusb-vhid-test job) until rawgadget unplug/replug exists
Hotplug_winapi builds + self-skips (incl. in the dedicated win-vhid-test job) until driver-side presence toggling exists
Hotplug_darwin builds + self-skips; local-only on an entitled Mac, like DeviceIO_darwin

⚠️ Expected to be red until the backend PRs land

The behavioral expectations encode the async-ENUMERATE contract, i.e. they require #822, #823, #824 and #825. On connection-callback as-is, tier 1 fails (e.g. *callback_handle is not zeroed on a failed registration yet), so this PR's CI will be red until those four PRs land — that is expected and is the reason this PR is a draft.

Validation ahead of that: an integration branch (hotplug-integration = connection-callback + all four backend PRs + these tests) runs the same CI; there, HotplugAPI_hidraw/HotplugAPI_libusb pass in a local WSL run of both backends and HotplugAPI_winapi passes locally on Windows/MSVC.

Relates to: #764

Assisted-by: claude-code:claude-fable-5

Two new test tiers for the hotplug API:

- test_hotplug_api.c (HotplugAPI_<backend>): argument validation,
  callback-handle properties, implicit init, hid_exit() teardown and
  register/deregister thread churn. Needs no device or privileges, so
  it runs against every backend in the ordinary CI matrix.

- test_hotplug.c (Hotplug_<backend>): device-backed hotplug scenarios
  (async delivery, exactly-once ENUMERATE pass, callback-return
  deregistration, pass-before-live ordering, ARRIVED/LEFT payloads,
  filtering, dispatch order, deregistration post-condition and
  re-entrant registration) against a virtual device whose presence is
  toggled with the new test_virtual_device_unplug()/_replug() calls.
  Implemented for the uhid provider (UHID_DESTROY/UHID_CREATE2 on the
  same fd); the other providers return TEST_VDEV_UNAVAILABLE and their
  hotplug tests self-skip until presence toggling is implemented.

Assisted-by: claude-code:claude-fable-5
Make the tier-2 Hotplug_<backend> test drive real plug/unplug on two more
providers so it exercises the async backends instead of self-skipping.

libusb (raw_gadget VM): the provider now unbinds/rebinds the gadget from the
dummy UDC (close the raw-gadget fd to disconnect; reopen + INIT + RUN to
reconnect), producing real libusb LEFT/ARRIVED events.

Windows (vhidmini UMDF driver): the provider locates the root devnode by its INF
hardware id (the instance path is PnP-derived from the setup class, not knowable
a priori) and toggles presence by disabling/enabling the child HID PDO, so the
UMDF host stays up and the HID interface cleanly drops and returns. The device
identity is aligned with the static driver (primary PID 0x9001 and serial
"HIDAPI-HOTPLUG-TEST" on Windows; other platforms keep 0x9002), and create()
reports UNAVAILABLE for a device that is not present so the second-device
sub-test skips. cfgmgr32 is linked for the winapi provider targets.

Both run only under the ci-virtual-device label (raw_gadget VM / installed
driver) and skip cleanly elsewhere.

Assisted-by: claude-code:claude-opus-4-8
@mcuee mcuee added the Test Related to built-in test (not hidtest) label Jul 17, 2026
@Youw

Youw commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Full implementation review (fresh pass, head 6753c0b)

Reviewed the T6–T15 device-backed scenarios and tier-1 contract tests against the documented contract in hidapi/hidapi.h, all four virtual-device providers (uhid, rawgadget, win/vhidmini, mac stub), the CMake wiring, and the two label-gated workflows. Each candidate finding was adversarially self-checked before being reported. The suite is well designed — barrier-based absence assertions, deadline-polled waits, per-handle event log, and skip semantics are sound, and every T6–T15 scenario genuinely proves what its name claims. Findings: 1 major, 5 minor, 1 nit. No blockers. (Note: this PR's CI is red only because its base still has the pre-async backends; the tests are verified green on the aggregate branch with PRs #822#825 included.)

[major] src/tests/test_hotplug.c:358-376, 912-921 — T15: acted flag published before the results it gates

cb_parent sets ctx->acted = 1 under g_log_lock (359-363) before it performs the child registration and self-deregistration; ctx->child_rc / ctx->child_handle / ctx->self_dereg_rc are stored only afterwards in a second locked section (372-376). The main thread's hp_wait_flag(&ctx.acted, ...) (912) can observe acted == 1, take the lock, and read the still-zero results while the event thread is between the two critical sections — a window that includes a full hid_hotplug_register_callback(... ENUMERATE ...) (snapshot construction) plus a deregistration, i.e. potentially milliseconds on a loaded CI host. CHECK(h_child > 0) (920) then fails spuriously on a fully compliant backend.

Suggested fix: store child_rc/child_handle/self_dereg_rc first and set a separate done flag last (same locked section); wait on done; keep acted only as the run-once guard.

[minor] src/tests/test_hotplug_api.c:188 — T2 asserts handle monotonicity, contract only promises non-reuse

CHECK(h2 > h1)hidapi.h promises handles are unique, positive, and not reused while initialized; it never promises a later registration gets a numerically larger handle. A compliant backend with, e.g., randomized handles would fail this contract test. Fix: relax to CHECK(h2 != h1) (keeping h2 > 0), or make monotonicity contractual in hidapi.h.

[minor] src/tests/test_virtual_device_rawgadget.c:651-668rg_unplug can join a still-blocked int_in thread with no further signal

The SIGUSR1 spin terminates as soon as ep0_exited is set. The int_in thread is only signalled inside that loop; if it is blocked in the blocking EP_WRITE ioctl (a queued input the host never read — precisely the failed-test case) it may never receive a signal, and the unconditional pthread_join (662) hangs until the CTest timeout kills the process, converting a clean failure into a 300 s timeout. Fix: gate the spin on both threads having exited (add int_in_exited), or keep signalling int_in until a timed join succeeds.

[minor] src/tests/test_virtual_device_rawgadget.c:682-702rg_plug has no already-plugged guard

test_virtual_device_replug() calls rg_plug() unconditionally, and rg_plug resets stop/fd/ep0_started/... without checking the device is actually unplugged. ensure_present() in test_hotplug.c decides to replug based on enumeration visibility, not plug state, so a plugged-but-not-enumerating device leads to rg_plug on a live device: the old fd leaks and the two orphaned worker threads keep running with stop cleared and fd retargeted — fd/thread state corruption. Fix: begin rg_plug with if (dev->fd >= 0 || dev->ep0_started) rg_unplug(dev); (documented idempotent), making replug safe from any state.

[minor] src/tests/test_virtual_device_win.c:188-193 — a crashed run that left the HID child disabled turns later local runs into silent skips

create() probes hid_enumerate and returns UNAVAILABLE when empty. destroy() best-effort re-enables the child, but a process killed by a CTest timeout — plausible mid-hotplug-test, exactly when the child is disabled — never runs destroy. The next local run finds nothing and skips (exit 77) even though the driver is installed, masking the tests until the device is re-enabled manually. (CI is unaffected: fresh runner each run.) Fix: in create(), before the probe, best-effort locate_vhid_devnode() + find_hid_child() + CM_Enable_DevNode, then probe with a brief re-poll.

[minor] src/tests/CMakeLists.txt:77,113-114,121, src/tests/README.md, .github/workflows/libusb-vhid-test.yml:67 — stale docs contradict the toggling this PR implements

CMake comments still say only uhid implements presence toggling and that the rawgadget/vhidmini tests "self-skip until implemented"; the README tier-2 table marks Hotplug_libusb/Hotplug_winapi as future; the VM workflow step is still named "Run DeviceIO_libusb inside a VM". All are contradicted by this PR's own rawgadget rg_plug/rg_unplug and Windows child-PDO toggling, which the label-gated jobs actually run. Fix: update the three CMake comments, the README paragraph/table, and the workflow step name.

[nit] test_hotplug_api.c:276, test_virtual_device_uhid.c:47, test_virtual_device_rawgadget.c:180-189 — cross-thread flags are volatile int, not atomics

Correct in practice for these one-way flags and invisible to ASan, but formal data races a future TSan leg would report. Fix: C11 atomic_int or the __atomic_* helpers test_platform.h already uses.


Verified sound (checked, no issues): T6–T14 assertion logic against the contract (exactly-once with LEFT barriers, return-stops-pass with fresh-ENUMERATE barrier, pass-before-live path correlation, zero-window T11 proof, registration-order absence barriers, race-free one-sided T14); event-log machinery (deep-copy under mutex, seq == dispatch order, per-test invariant sweeps, VID/PID/serial filtering); complete skip semantics (exit 77 on no-hotplug, provider-unavailable, no-toggle, never-enumerates, non-elevated Windows; T8b second-device sub-skip); uhid provider lifecycle; Windows devnode location (ROOT-enumerator scan by INF hardware id) and child-PDO toggling; CMake targets/linkage/timeouts; label gating and driver artifact flow in both workflows.

Drafted with Claude Code.

Youw added 2 commits August 4, 2026 19:59
Publish T15's results before the flag the main thread waits on, relax the
handle-uniqueness assertion to match the documented contract, make the
rawgadget teardown signal both workers until both exit and guard rg_plug
against a redundant plug, re-enable a left-disabled HID child before the
Windows provider's presence probe, and refresh the docs that still said
only uhid can toggle presence.

Assisted-by: claude-code:claude-opus-5
The already-plugged guard added in the previous commit mistook the
calloc'd fd 0 for an open gadget fd, so the first plug closed the
process's stdin; initialise fd to -1 before rg_plug() sees it. Also set
ep0_exited on ep0_thread_fn's allocation-failure return, so teardown
does not spend its whole signalling budget on a thread that already left.

Assisted-by: claude-code:claude-opus-5
@Youw

Youw commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Re-review of fix commits aaf66c0 + c54ecc6

A fresh delta review (independent reviewer) verified aaf66c0 against all six findings and hunted for newly introduced defects:

  • T15 publish ordering — FIXED. Single locked section stores the results then sets done=1; the main thread waits on done (deadline-bounded, read under the lock); acted remains the run-once guard, so a second ARRIVED can neither double-store nor skip storing.
  • Handle-uniqueness assertion — FIXED (h2 != h1; no other monotonicity assumptions remain in either test file).
  • rawgadget teardown — FIXED. int_in_exited is set on the thread's single exit path, reset in the per-plug block, and the signal spin now requires both workers to have exited, still signalling each busy thread independently.
  • replug guard — FIXED as specified, but the guard itself introduced 1 new defect (below).
  • Windows create() re-enable + re-poll — FIXED. Helpers are defined above create(); every call site was checked — T8b's second-device create runs after ensure_present(), so the pre-probe re-enable is a no-op there, and no call site exists with the primary intentionally unplugged. (Noted behavior change: an absent device now costs ~3 s before UNAVAILABLE instead of an instant skip — bounded, acceptable against the 300 s test timeout.)
  • Stale docs — FIXED and verified against what the workflows actually run.

New defect found by the re-review (introduced by the new guard, then fixed in c54ecc6): test_virtual_device_create() callocs the device struct, leaving fd == 0, and the new already-plugged guard treated fd 0 as an open gadget fd — so every rawgadget create() deterministically closed stdin (close(0)), masked only because open("/dev/raw-gadget") then recycled fd 0. c54ecc6 puts the struct in the documented unplugged state (fd = -1) before rg_plug() inspects it, and also closes a pre-existing OOM-only gap the re-review surfaced (ep0_thread_fn's early return never published ep0_exited, which would have made rg_unplug spin its full signal budget against an already-dead thread).

Both follow-up hunks were verified against the re-review's prescription; gcc -fsyntax-only passes against the real raw_gadget.h UAPI header.

Drafted with Claude Code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Test Related to built-in test (not hidtest)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants