From 93fca95d4780c5ebd31883c9aa8dc71e59ef3ae4 Mon Sep 17 00:00:00 2001 From: BANANASJIM Date: Sat, 6 Jun 2026 21:03:25 -0700 Subject: [PATCH] refactor(io): address audit findings (dedup, dead-code, comment hygiene) - uhid: drop unreachable `if (n < 4)` branch in pollOutputReport (short reads already return error.IncompleteUhidEvent) - uhid: replace redundant @min/copy_len pad-copy in uhidCreate, uhidInput, and sendCreate with a direct @memcpy guarded by a comptime size assert - uhid_descriptor: trim implemented forward-compat comment on INPUT_REPORT_ID; drop TODO marker from skipped buildForPid golden-fixture test, rename it - usbraw: extract libusbOpenAndClaim helper shared by UsbrawDevice.open and UsbrawSuppress.openSuppress (was duplicated init/open/detach/claim sequence) - netlink: collapse duplicate WouldBlock/else recv error arms into catch return refs: codebase audit --- src/io/netlink.zig | 5 +-- src/io/uhid.zig | 19 ++++----- src/io/uhid_descriptor.zig | 9 +--- src/io/usbraw.zig | 87 +++++++++++++++++--------------------- 4 files changed, 50 insertions(+), 70 deletions(-) diff --git a/src/io/netlink.zig b/src/io/netlink.zig index 777cea6f..5665a1de 100644 --- a/src/io/netlink.zig +++ b/src/io/netlink.zig @@ -62,10 +62,7 @@ pub fn parseUevent(buf: []const u8) Uevent { pub fn drainNetlink(fd: posix.fd_t, ctx: anytype, comptime callback: fn (@TypeOf(ctx), UeventAction, []const u8) void) void { var buf: [2048]u8 = undefined; while (true) { - const n = posix.recv(fd, &buf, 0) catch |err| switch (err) { - error.WouldBlock => return, - else => return, - }; + const n = posix.recv(fd, &buf, 0) catch return; if (n == 0) return; const ev = parseUevent(buf[0..n]); if (ev.action == .other) continue; diff --git a/src/io/uhid.zig b/src/io/uhid.zig index 9d39cf6c..afe0db21 100644 --- a/src/io/uhid.zig +++ b/src/io/uhid.zig @@ -116,10 +116,10 @@ pub fn uhidCreate( ev.payload.country = 0; @memcpy(ev.payload.rd_data[0..rd_data.len], rd_data); - const bytes = std.mem.asBytes(&ev); + comptime std.debug.assert(@sizeOf(UhidCreate2Event) <= UHID_EVENT_SIZE); var buf: [UHID_EVENT_SIZE]u8 = std.mem.zeroes([UHID_EVENT_SIZE]u8); - const copy_len = @min(bytes.len, UHID_EVENT_SIZE); - @memcpy(buf[0..copy_len], bytes[0..copy_len]); + const bytes = std.mem.asBytes(&ev); + @memcpy(buf[0..bytes.len], bytes); try write_exact.writeExact(fd, &buf); } @@ -136,10 +136,10 @@ pub fn uhidInput(fd: posix.fd_t, data: []const u8) !void { ev.payload.size = @intCast(data.len); @memcpy(ev.payload.data[0..data.len], data); - const bytes = std.mem.asBytes(&ev); + comptime std.debug.assert(@sizeOf(UhidInput2Event) <= UHID_EVENT_SIZE); var buf: [UHID_EVENT_SIZE]u8 = std.mem.zeroes([UHID_EVENT_SIZE]u8); - const copy_len = @min(bytes.len, UHID_EVENT_SIZE); - @memcpy(buf[0..copy_len], bytes[0..copy_len]); + const bytes = std.mem.asBytes(&ev); + @memcpy(buf[0..bytes.len], bytes); try write_exact.writeExact(fd, &buf); } @@ -346,10 +346,10 @@ pub const UhidDevice = struct { ev.payload.country = cfg.country; @memcpy(ev.payload.rd_data[0..cfg.descriptor.len], cfg.descriptor); - const bytes = std.mem.asBytes(&ev); + comptime std.debug.assert(@sizeOf(UhidCreate2Event) <= UHID_EVENT_SIZE); var buf: [UHID_EVENT_SIZE]u8 = std.mem.zeroes([UHID_EVENT_SIZE]u8); - const copy_len = @min(bytes.len, UHID_EVENT_SIZE); - @memcpy(buf[0..copy_len], bytes[0..copy_len]); + const bytes = std.mem.asBytes(&ev); + @memcpy(buf[0..bytes.len], bytes); write_exact.writeExact(fd, &buf) catch return error.UhidCreateFailed; } @@ -459,7 +459,6 @@ pub const UhidDevice = struct { // field lives past offset 4100, so even a read of 4095 bytes would let us // parse a garbage size and walk off the end of data[]. if (n < UHID_EVENT_SIZE) return error.IncompleteUhidEvent; - if (n < 4) return null; const ev_type = std.mem.readInt(u32, buf[0..4], .little); if (ev_type != UHID_OUTPUT) return null; // kernel struct uhid_event: { u32 type; union payload; } diff --git a/src/io/uhid_descriptor.zig b/src/io/uhid_descriptor.zig index 24b9a8a1..15c7b60a 100644 --- a/src/io/uhid_descriptor.zig +++ b/src/io/uhid_descriptor.zig @@ -100,9 +100,7 @@ const PID_MANDATORY_REPORT_IDS = [_]u8{ PID_POOL_REPORT_ID, // 15 }; -/// Input report ID used for the main gamepad report. Kept `1` so a simple -/// gamepad (no output/feature reports) could legally omit the ID prefix; we -/// still emit it for forward-compat with FFB output reports. +/// Input report ID for the main gamepad report. pub const INPUT_REPORT_ID: u8 = 1; /// Output report ID used for the FFB rumble output report. 2 byte payload @@ -2072,10 +2070,7 @@ test "buildForPid: stays within HID_MAX_DESCRIPTOR_SIZE" { try testing.expect(desc.len <= uhid.HID_MAX_DESCRIPTOR_SIZE); } -// TODO: pin the byte sequence emitted by buildForPid against a known-good -// reference once real-hardware validates kernel `hid-universal-pidff` FFB init -// (no `pidff_find_reports -ENODEV`). Until then this test is intentionally skipped. -test "buildForPid: matches reference PID descriptor (Moza R5)" { +test "buildForPid: golden fixture pending real-hw validation" { return error.SkipZigTest; } diff --git a/src/io/usbraw.zig b/src/io/usbraw.zig index a909eaf2..a475ba97 100644 --- a/src/io/usbraw.zig +++ b/src/io/usbraw.zig @@ -67,6 +67,35 @@ pub const RingBuffer = struct { } }; +// Init libusb, open the device, detach any kernel driver, and claim the +// interface. On any failure the partially-acquired state is rolled back and +// the error is returned; on success the caller owns ctx + handle and the +// claimed interface. +fn libusbOpenAndClaim(vid: u16, pid: u16, interface_id: u8) !struct { + ctx: *c.libusb_context, + handle: *c.libusb_device_handle, +} { + var ctx: ?*c.libusb_context = null; + if (c.libusb_init(&ctx) != 0) return error.LibusbInit; + + const handle = c.libusb_open_device_with_vid_pid(ctx, vid, pid) orelse { + c.libusb_exit(ctx); + return error.NotFound; + }; + + _ = c.libusb_detach_kernel_driver(handle, interface_id); + + const rc = c.libusb_claim_interface(handle, interface_id); + if (rc != 0) { + _ = c.libusb_attach_kernel_driver(handle, interface_id); + c.libusb_close(handle); + c.libusb_exit(ctx); + return if (rc == c.LIBUSB_ERROR_BUSY) error.Busy else error.ClaimFailed; + } + + return .{ .ctx = ctx.?, .handle = handle }; +} + pub const UsbrawDevice = struct { handle: *c.libusb_device_handle, ctx: *c.libusb_context, @@ -89,29 +118,9 @@ pub const UsbrawDevice = struct { ep_in: u8, ep_out: u8, ) !*UsbrawDevice { - var ctx: ?*c.libusb_context = null; - if (c.libusb_init(&ctx) != 0) return error.LibusbInit; - - const handle = c.libusb_open_device_with_vid_pid(ctx, vid, pid) orelse { - c.libusb_exit(ctx); - return error.NotFound; - }; - - _ = c.libusb_detach_kernel_driver(handle, interface_id); - - const rc = c.libusb_claim_interface(handle, interface_id); - if (rc == c.LIBUSB_ERROR_BUSY) { - _ = c.libusb_attach_kernel_driver(handle, interface_id); - c.libusb_close(handle); - c.libusb_exit(ctx); - return error.Busy; - } - if (rc != 0) { - _ = c.libusb_attach_kernel_driver(handle, interface_id); - c.libusb_close(handle); - c.libusb_exit(ctx); - return error.ClaimFailed; - } + const claimed = try libusbOpenAndClaim(vid, pid, interface_id); + const ctx = claimed.ctx; + const handle = claimed.handle; // Claim succeeded; release it and restore the kernel driver on any later // failure so the interface is never leaked claimed-but-unowned. @@ -119,7 +128,7 @@ pub const UsbrawDevice = struct { _ = c.libusb_release_interface(handle, interface_id); _ = c.libusb_attach_kernel_driver(handle, interface_id); c.libusb_close(handle); - c.libusb_exit(ctx.?); + c.libusb_exit(ctx); } const pipe_fds = try std.posix.pipe2(.{ .NONBLOCK = true, .CLOEXEC = true }); @@ -130,7 +139,7 @@ pub const UsbrawDevice = struct { errdefer alloc.destroy(self); self.* = .{ .handle = handle, - .ctx = ctx.?, + .ctx = ctx, .ep_in = ep_in, .ep_out = ep_out, .interface_id = @intCast(interface_id), @@ -262,29 +271,9 @@ pub const UsbrawSuppress = struct { pid: u16, interface_id: u8, ) !*UsbrawSuppress { - var ctx: ?*c.libusb_context = null; - if (c.libusb_init(&ctx) != 0) return error.LibusbInit; - - const handle = c.libusb_open_device_with_vid_pid(ctx, vid, pid) orelse { - c.libusb_exit(ctx); - return error.NotFound; - }; - - _ = c.libusb_detach_kernel_driver(handle, interface_id); - - const rc = c.libusb_claim_interface(handle, interface_id); - if (rc == c.LIBUSB_ERROR_BUSY) { - _ = c.libusb_attach_kernel_driver(handle, interface_id); - c.libusb_close(handle); - c.libusb_exit(ctx); - return error.Busy; - } - if (rc != 0) { - _ = c.libusb_attach_kernel_driver(handle, interface_id); - c.libusb_close(handle); - c.libusb_exit(ctx); - return error.ClaimFailed; - } + const claimed = try libusbOpenAndClaim(vid, pid, interface_id); + const ctx = claimed.ctx; + const handle = claimed.handle; const self = alloc.create(UsbrawSuppress) catch |err| { _ = c.libusb_release_interface(handle, interface_id); @@ -295,7 +284,7 @@ pub const UsbrawSuppress = struct { }; self.* = .{ .handle = handle, - .ctx = ctx.?, + .ctx = ctx, .interface_id = @intCast(interface_id), .allocator = alloc, };