From 7dc584f3836e51607af905fed1ed1db1926b619f Mon Sep 17 00:00:00 2001 From: BANANASJIM Date: Sat, 6 Jun 2026 22:00:34 -0700 Subject: [PATCH] refactor(cli): inject allocators + de-flake/extend tests (audit follow-ups) - config/test.zig readVidPid: take allocator param instead of page_allocator; thread through from run() (api-consistency) - switch_mapping.zig run(): accept allocator instead of building an internal GPA; thread the process allocator from main.zig - devices.zig test: bind+listen on caller thread before spawning server, removing Thread.sleep(10ms) race; assertion unchanged - config/test.zig: extract formatReport helper and add falsifiable Layer-0 tests for the report hex-dump, remap hint, and readVidPid refs: codebase audit --- src/cli/config/test.zig | 90 ++++++++++++++++++++++++++------------ src/cli/devices.zig | 27 +++++++----- src/cli/switch_mapping.zig | 7 +-- src/main.zig | 2 +- 4 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/cli/config/test.zig b/src/cli/config/test.zig index 73b2d52a..a191ac59 100644 --- a/src/cli/config/test.zig +++ b/src/cli/config/test.zig @@ -20,9 +20,9 @@ const HIDIOCGRAWNAME: u32 = blk: { break :blk @as(u32, @bitCast(req)); }; -fn readVidPid(config_path: []const u8) !struct { vid: u16, pid: u16 } { - const content = try std.fs.cwd().readFileAlloc(std.heap.page_allocator, config_path, 256 * 1024); - defer std.heap.page_allocator.free(content); +fn readVidPid(allocator: std.mem.Allocator, config_path: []const u8) !struct { vid: u16, pid: u16 } { + const content = try std.fs.cwd().readFileAlloc(allocator, config_path, 256 * 1024); + defer allocator.free(content); const vid = scan_mod.extractHexField(content, "vid") orelse return error.MissingVid; const pid = scan_mod.extractHexField(content, "pid") orelse return error.MissingPid; return .{ .vid = vid, .pid = pid }; @@ -73,7 +73,7 @@ pub fn run(allocator: std.mem.Allocator, config_path: ?[]const u8, mapping_path: const fd = blk: { if (config_path) |cp| { - const vp = readVidPid(cp) catch |e| { + const vp = readVidPid(allocator, cp) catch |e| { std.log.err("failed to read VID/PID from '{s}': {}", .{ cp, e }); return e; }; @@ -115,36 +115,36 @@ pub fn run(allocator: std.mem.Allocator, config_path: ?[]const u8, mapping_path: if (n == 0) break; out.clearRetainingCapacity(); - try w.print("report[{d}B]:", .{n}); - for (report_buf[0..n]) |byte| { - try w.print(" {x:0>2}", .{byte}); - } + try formatReport(w, report_buf[0..n], if (mapping) |m| m.value.remap else null); + writer.writeAll(out.items) catch {}; + } +} + +fn formatReport(w: anytype, report: []const u8, remap: ?mapping_mod.RemapMap) !void { + try w.print("report[{d}B]:", .{report.len}); + for (report) |byte| { + try w.print(" {x:0>2}", .{byte}); + } - if (mapping) |m| { - // Show any known remaps as a hint - const remap = m.value.remap; - if (remap != null) { - var it = remap.?.map.iterator(); - while (it.next()) |entry| { - switch (entry.value_ptr.*) { - .string => |s| try w.print(" {s} -> {s}", .{ entry.key_ptr.*, s }), - .chord_names => |names| { - try w.print(" {s} -> chord[", .{entry.key_ptr.*}); - for (names, 0..) |name, i| { - if (i > 0) try w.writeAll(", "); - try w.print("{s}", .{name}); - } - try w.writeAll("]"); - }, - .gesture => try w.print(" {s} -> ", .{entry.key_ptr.*}), + if (remap) |r| { + var it = r.map.iterator(); + while (it.next()) |entry| { + switch (entry.value_ptr.*) { + .string => |s| try w.print(" {s} -> {s}", .{ entry.key_ptr.*, s }), + .chord_names => |names| { + try w.print(" {s} -> chord[", .{entry.key_ptr.*}); + for (names, 0..) |name, i| { + if (i > 0) try w.writeAll(", "); + try w.print("{s}", .{name}); } - } + try w.writeAll("]"); + }, + .gesture => try w.print(" {s} -> ", .{entry.key_ptr.*}), } } - - try w.writeByte('\n'); - writer.writeAll(out.items) catch {}; } + + try w.writeByte('\n'); } // --- tests --- @@ -152,3 +152,35 @@ pub fn run(allocator: std.mem.Allocator, config_path: ?[]const u8, mapping_path: // "openFirstHidraw returns error when no device" test removed: openFirstHidraw // scans /dev/hidraw0..63 and on dev machines an orphaned UHID device causes // hid_hw_open D-state even with O_NONBLOCK. The function is exercised via run(). + +const testing = std.testing; + +test "formatReport: hex dump without mapping" { + var out: std.ArrayList(u8) = .{}; + defer out.deinit(testing.allocator); + try formatReport(out.writer(testing.allocator), &[_]u8{ 0x01, 0xab, 0x00 }, null); + try testing.expectEqualStrings("report[3B]: 01 ab 00\n", out.items); +} + +test "formatReport: appends remap hint" { + var map = std.StringHashMap(mapping_mod.RemapValue).init(testing.allocator); + defer map.deinit(); + try map.put("BTN_SOUTH", .{ .string = "KEY_A" }); + + var out: std.ArrayList(u8) = .{}; + defer out.deinit(testing.allocator); + try formatReport(out.writer(testing.allocator), &[_]u8{0xff}, .{ .map = map }); + try testing.expectEqualStrings("report[1B]: ff BTN_SOUTH -> KEY_A\n", out.items); +} + +test "readVidPid: parses vid/pid from device toml" { + var tmp = testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.writeFile(.{ .sub_path = "dev.toml", .data = "vid = 0x045e\npid = 0x028e\n" }); + + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const path = try tmp.dir.realpath("dev.toml", &path_buf); + const vp = try readVidPid(testing.allocator, path); + try testing.expectEqual(@as(u16, 0x045e), vp.vid); + try testing.expectEqual(@as(u16, 0x028e), vp.pid); +} diff --git a/src/cli/devices.zig b/src/cli/devices.zig index 45f4d8a4..7a0b6f00 100644 --- a/src/cli/devices.zig +++ b/src/cli/devices.zig @@ -28,20 +28,27 @@ pub fn run(socket_path: []const u8, writer: anytype, err_writer: anytype) u8 { const testing = std.testing; const TestServer = struct { - socket_path: []const u8, + listen_fd: posix.fd_t, response: []const u8, - fn run(ctx: *@This()) void { - const listen_fd = posix.socket(posix.AF.UNIX, posix.SOCK.STREAM | posix.SOCK.CLOEXEC, 0) catch return; - defer posix.close(listen_fd); + // Bind + listen on the caller's thread so the socket file exists before the + // client connects, eliminating the connect/bind race. The accept loop runs + // on the spawned thread. + fn bind(socket_path: []const u8) !posix.fd_t { + const listen_fd = try posix.socket(posix.AF.UNIX, posix.SOCK.STREAM | posix.SOCK.CLOEXEC, 0); + errdefer posix.close(listen_fd); var addr: std.os.linux.sockaddr.un = .{ .family = posix.AF.UNIX, .path = undefined }; @memset(&addr.path, 0); - @memcpy(addr.path[0..ctx.socket_path.len], ctx.socket_path); - posix.bind(listen_fd, @ptrCast(&addr), @sizeOf(std.os.linux.sockaddr.un)) catch return; - posix.listen(listen_fd, 1) catch return; + @memcpy(addr.path[0..socket_path.len], socket_path); + try posix.bind(listen_fd, @ptrCast(&addr), @sizeOf(std.os.linux.sockaddr.un)); + try posix.listen(listen_fd, 1); + return listen_fd; + } - const client_fd = posix.accept(listen_fd, null, null, 0) catch return; + fn run(ctx: *@This()) void { + defer posix.close(ctx.listen_fd); + const client_fd = posix.accept(ctx.listen_fd, null, null, 0) catch return; defer posix.close(client_fd); _ = posix.write(client_fd, ctx.response) catch {}; } @@ -59,14 +66,12 @@ test "run: ERR response returns 1" { const sock_path = try std.fmt.bufPrint(&sock_path_buf, "{s}/devices.sock", .{tmp_path}); var server = TestServer{ - .socket_path = sock_path, + .listen_fd = try TestServer.bind(sock_path), .response = "ERR device unavailable\n", }; const thread = try std.Thread.spawn(.{}, TestServer.run, .{&server}); defer thread.join(); - std.Thread.sleep(10 * std.time.ns_per_ms); - const rc = run(sock_path, std.io.null_writer, std.io.null_writer); try testing.expectEqual(@as(u8, 1), rc); } diff --git a/src/cli/switch_mapping.zig b/src/cli/switch_mapping.zig index ed6a108d..91f7570e 100644 --- a/src/cli/switch_mapping.zig +++ b/src/cli/switch_mapping.zig @@ -3,7 +3,7 @@ const posix = std.posix; const socket_client = @import("socket_client.zig"); const mapping_discovery = @import("../config/mapping_discovery.zig"); -pub fn run(name: []const u8, device_id: ?[]const u8, socket_path: []const u8, writer: anytype, err_writer: anytype) u8 { +pub fn run(allocator: std.mem.Allocator, name: []const u8, device_id: ?[]const u8, socket_path: []const u8, writer: anytype, err_writer: anytype) u8 { const fd = socket_client.connectToSocket(socket_path) catch { err_writer.writeAll("error: cannot connect to padctl daemon\n") catch {}; return 1; @@ -13,9 +13,6 @@ pub fn run(name: []const u8, device_id: ?[]const u8, socket_path: []const u8, wr // Resolve mapping path client-side so the user's ~/.config/padctl/mappings/ // takes priority over /etc/padctl/mappings/. Send the full path to the daemon // which accepts both names and absolute paths. - var gpa = std.heap.GeneralPurposeAllocator(.{}){}; - defer _ = gpa.deinit(); - const allocator = gpa.allocator(); const resolved = mapping_discovery.findMapping(allocator, name) catch null; defer if (resolved) |p| allocator.free(p); const switch_name = resolved orelse name; @@ -46,6 +43,6 @@ pub fn run(name: []const u8, device_id: ?[]const u8, socket_path: []const u8, wr const testing = std.testing; test "run: connection failure returns 1" { - const rc = run("fps", null, "/tmp/padctl-nonexistent-test.sock", std.io.null_writer, std.io.null_writer); + const rc = run(testing.allocator, "fps", null, "/tmp/padctl-nonexistent-test.sock", std.io.null_writer, std.io.null_writer); try testing.expectEqual(@as(u8, 1), rc); } diff --git a/src/main.zig b/src/main.zig index 39be91ef..8f41c898 100644 --- a/src/main.zig +++ b/src/main.zig @@ -943,7 +943,7 @@ pub fn main() !void { break :blk resolveDefaultMapping(allocator, parsed.socket_path, stderr_writer); }; - const rc = cli.switch_mapping.run(mapping_name, sw.device_id, parsed.socket_path, stdout_writer, stderr_writer); + const rc = cli.switch_mapping.run(allocator, mapping_name, sw.device_id, parsed.socket_path, stdout_writer, stderr_writer); if (rc == 0) { // Auto-save to user config so `padctl switch` (no args) can // restore the choice. Skipped when --device targets a specific