Skip to content

Commit 88da2fe

Browse files
beriberikixclaude
andcommitted
Keep the bind list out of the configuration file
The list of bound devices lived in usbipd-config.json beside the port, the log level and the transfer tuning. The two have different owners: a port is chosen by a person and edited by hand, while a bind list is written by a command and read by a daemon. Holding them together had consequences beyond untidiness. `bind` rewrote the entire configuration file to append one busid. Any hand-edit made between the CLI reading that file and writing it back was lost. Worse, the CLI falls back to a default configuration when the file will not parse, so a single malformed character turned the next `bind` into a silent reset of the port and the log level — a command that shares a USB device quietly discarding unrelated settings, with a warning on stderr as the only sign. Appending was also unsynchronised. Read the list, add an entry, write it back: two `bind` commands doing that at once leave only one of the devices bound. Atomic writes prevent a reader seeing half a file; they do nothing about a lost update. BoundDeviceStore owns the list now, in bound-devices.json, and it is the only thing bind and unbind touch. The read-modify-write happens under an exclusive lock held on a separate lock file — separate because saving replaces the state file by rename, so a lock taken on the state file itself would leave two writers holding locks on two different inodes and both proceeding. Reads take no lock: writes are atomic, so a reader sees one whole version or the other, and locking every read would put a system call in front of every request for nothing. A list left in an old configuration file is adopted the first time the store is read. The old key is not removed — rewriting that file is the behaviour this change exists to stop — and nothing reads it afterwards. Verified against a Raspberry Pi Debug Probe. The configuration file is byte-identical across a bind and an unbind. A deliberately corrupted configuration is left untouched by a bind that still succeeds, so a hand-written port survives where it used to be reset. A pre-existing allowedDevices entry migrates and the device attaches with no re-bind. Forty concurrent binds all survive, and removing the lock loses them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017rmyikdjWveP99ZUCDLY89
1 parent 856262d commit 88da2fe

14 files changed

Lines changed: 496 additions & 269 deletions

CLAUDE.md

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,19 @@ plugged straight into the Mac, because no target is wired to its SWD pins. No Sy
2222
Extension and no entitlement are involved.
2323

2424
`bind` and `unbind` take effect on a running daemon. They are separate processes that
25-
write `~/.usbipd/usbipd-config.json`; the daemon notices the file has changed and
26-
re-reads the allow-list. It used to read that file only at startup, so a device bound
27-
while the daemon was running stayed unimportable — `usbip attach` answered "Request
28-
Failed" — and nothing said a restart was needed. Only the allow-list is re-read; port
29-
and log level still need a restart.
25+
write `~/.usbipd/bound-devices.json`; the daemon notices the file has changed and
26+
re-reads it. It used to read that state only at startup, so a device bound while the
27+
daemon was running stayed unimportable — `usbip attach` answered "Request Failed" — and
28+
nothing said a restart was needed.
29+
30+
State and configuration are separate files, and only state is ever written by the tool.
31+
`~/.usbipd/usbipd-config.json` holds the port, log level and tuning; it is read at
32+
startup and needs a restart to change. `~/.usbipd/bound-devices.json` holds the bind
33+
list, guarded by a lock file so two `bind` commands cannot lose each other's device.
34+
The two used to share one file, so `bind` rewrote the whole configuration to append a
35+
busid — and because the CLI falls back to defaults when that file will not parse, one
36+
malformed character turned the next `bind` into a silent reset of the port and log
37+
level. A `allowedDevices` key left in an old config is migrated once and then ignored.
3038

3139
Clients that cancel a transfer are supported, which matters more than it sounds:
3240
libusb cancels any read it has timed out, and probe-rs drains the IN endpoint that way

Sources/USBIPDCLI/CommandLineParser.swift

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,13 +52,22 @@ public class CommandLineParser {
5252

5353
/// System Extension manager for device claiming operations
5454
private let systemExtensionManager: SystemExtensionManager?
55-
55+
56+
/// Which devices are bound for sharing. Separate from `serverConfig` because bind
57+
/// and unbind write it, while the configuration is only ever read.
58+
private let boundDevices: BoundDeviceStore
59+
5660
/// Initialize a new command-line parser with dependencies
57-
public init(deviceDiscovery: DeviceDiscovery, serverConfig: ServerConfig, server: USBIPServer, systemExtensionManager: SystemExtensionManager? = nil) {
61+
public init(deviceDiscovery: DeviceDiscovery,
62+
serverConfig: ServerConfig,
63+
server: USBIPServer,
64+
systemExtensionManager: SystemExtensionManager? = nil,
65+
boundDevices: BoundDeviceStore = BoundDeviceStore()) {
5866
self.deviceDiscovery = deviceDiscovery
5967
self.serverConfig = serverConfig
6068
self.server = server
6169
self.systemExtensionManager = systemExtensionManager
70+
self.boundDevices = boundDevices
6271
registerCommands()
6372
}
6473

@@ -69,8 +78,8 @@ public class CommandLineParser {
6978
let commands: [Command] = [
7079
HelpCommand(parser: self),
7180
ListCommand(deviceDiscovery: deviceDiscovery, outputFormatter: outputFormatter),
72-
BindCommand(deviceDiscovery: deviceDiscovery, serverConfig: serverConfig, systemExtensionManager: systemExtensionManager),
73-
UnbindCommand(deviceDiscovery: deviceDiscovery, serverConfig: serverConfig, systemExtensionManager: systemExtensionManager),
81+
BindCommand(deviceDiscovery: deviceDiscovery, boundDevices: boundDevices, systemExtensionManager: systemExtensionManager),
82+
UnbindCommand(deviceDiscovery: deviceDiscovery, boundDevices: boundDevices, systemExtensionManager: systemExtensionManager),
7483
StatusCommand(deviceClaimManager: nil, outputFormatter: outputFormatter),
7584
// attach/detach are deliberately not registered. usbipd is a USB/IP
7685
// *server*; attaching a remote device is the client's job, and both

Sources/USBIPDCLI/Commands.swift

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -164,16 +164,16 @@ public class BindCommand: Command {
164164
public let description = "Bind a USB device to USB/IP through System Extension"
165165

166166
private let deviceDiscovery: DeviceDiscovery
167-
private let serverConfig: ServerConfig
167+
private let boundDevices: BoundDeviceStore
168168
private let systemExtensionManager: SystemExtensionManager?
169169
private let ownershipInspector: DeviceOwnershipInspector
170170

171171
public init(deviceDiscovery: DeviceDiscovery,
172-
serverConfig: ServerConfig,
172+
boundDevices: BoundDeviceStore = BoundDeviceStore(),
173173
systemExtensionManager: SystemExtensionManager? = nil,
174174
ownershipInspector: DeviceOwnershipInspector = DeviceOwnershipInspector()) {
175175
self.deviceDiscovery = deviceDiscovery
176-
self.serverConfig = serverConfig
176+
self.boundDevices = boundDevices
177177
self.systemExtensionManager = systemExtensionManager
178178
self.ownershipInspector = ownershipInspector
179179
}
@@ -301,9 +301,8 @@ public class BindCommand: Command {
301301
logger.info("Device already claimed by System Extension", context: ["deviceID": deviceIdentifier])
302302
print("Device \(busid) is already claimed by System Extension")
303303

304-
// Add to config even if already claimed to ensure consistency
305-
serverConfig.allowDevice(deviceIdentifier)
306-
try serverConfig.save()
304+
// Record it even if already claimed, so the two agree.
305+
try boundDevices.bind(deviceIdentifier)
307306

308307
print("Device \(busid) successfully bound: \(String(format: "%04x", device.vendorID)):\(String(format: "%04x", device.productID)) (\(device.productString ?? "Unknown"))")
309308
return
@@ -345,13 +344,14 @@ public class BindCommand: Command {
345344
print(" work from userspace and need no claim.")
346345
}
347346

348-
// Step 3: Add device to allowed devices in config
349-
logger.debug("Adding device to allowed devices list", context: ["deviceIdentifier": deviceIdentifier])
350-
serverConfig.allowDevice(deviceIdentifier)
351-
352-
// Step 4: Save the updated configuration
353-
logger.debug("Saving updated configuration")
354-
try serverConfig.save()
347+
// Step 3: Record the device as bound.
348+
//
349+
// This writes only the bound-device list. It used to append to the server
350+
// configuration and write that whole file back, so sharing a USB device
351+
// rewrote the port, the log level and every tuning value along with it —
352+
// and reset them outright whenever the configuration had failed to parse.
353+
logger.debug("Recording the device as bound", context: ["deviceIdentifier": deviceIdentifier])
354+
try boundDevices.bind(deviceIdentifier)
355355

356356
logger.info("Successfully bound device", context: ["busid": busid])
357357
print("✓ Device \(busid) added to server configuration")
@@ -427,12 +427,14 @@ public class UnbindCommand: Command {
427427
public let description = "Unbind a USB device from USB/IP and release System Extension claim"
428428

429429
private let deviceDiscovery: DeviceDiscovery
430-
private let serverConfig: ServerConfig
430+
private let boundDevices: BoundDeviceStore
431431
private let systemExtensionManager: SystemExtensionManager?
432432

433-
public init(deviceDiscovery: DeviceDiscovery, serverConfig: ServerConfig, systemExtensionManager: SystemExtensionManager? = nil) {
433+
public init(deviceDiscovery: DeviceDiscovery,
434+
boundDevices: BoundDeviceStore = BoundDeviceStore(),
435+
systemExtensionManager: SystemExtensionManager? = nil) {
434436
self.deviceDiscovery = deviceDiscovery
435-
self.serverConfig = serverConfig
437+
self.boundDevices = boundDevices
436438
self.systemExtensionManager = systemExtensionManager
437439
}
438440

@@ -474,7 +476,7 @@ public class UnbindCommand: Command {
474476
logger.debug("Looking for device", context: ["busID": busPart, "deviceID": devicePart])
475477

476478
// Step 1: Check if device is currently bound in config
477-
let wasBound = serverConfig.allowedDevices.contains(busid)
479+
let wasBound = boundDevices.isBound(busid)
478480
logger.debug("Device binding status in config", context: ["busid": busid, "wasBound": wasBound])
479481

480482
// Step 2: Attempt to release device through System Extension if available
@@ -542,15 +544,12 @@ public class UnbindCommand: Command {
542544
print("Note: Device claiming through System Extension is not active")
543545
}
544546

545-
// Step 3: Remove device from allowed devices in config
546-
logger.debug("Removing device from allowed devices list", context: ["busid": busid])
547-
let removed = serverConfig.disallowDevice(busid)
547+
// Step 3: Forget the device.
548+
logger.debug("Removing the device from the bound list", context: ["busid": busid])
549+
let removed = try boundDevices.unbind(busid)
548550

549551
if removed {
550-
// Step 4: Save the updated configuration
551-
logger.debug("Saving updated configuration")
552-
try serverConfig.save()
553-
logger.info("Successfully unbound device from configuration", context: ["busid": busid])
552+
logger.info("Successfully unbound device", context: ["busid": busid])
554553
print("✓ Device \(busid) removed from server configuration")
555554

556555
if systemExtensionManager != nil {
@@ -692,7 +691,6 @@ public class DaemonCommand: Command {
692691
serverConfig.debugMode = loadedConfig.debugMode
693692
serverConfig.maxConnections = loadedConfig.maxConnections
694693
serverConfig.connectionTimeout = loadedConfig.connectionTimeout
695-
serverConfig.allowedDevices = loadedConfig.allowedDevices
696694
serverConfig.logFilePath = loadedConfig.logFilePath
697695

698696
logger.debug("Applied custom configuration to server")
Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
// BoundDeviceStore.swift
2+
// Which devices have been bound for sharing, kept apart from user configuration.
3+
4+
import Foundation
5+
import Common
6+
7+
/// The list of devices `bind` has made available, persisted so it survives a restart.
8+
///
9+
/// This used to live in the configuration file alongside the port, the log level and
10+
/// the transfer tuning, which was wrong in three ways.
11+
///
12+
/// The two have different owners. A port is chosen by a person and edited by hand; a
13+
/// bind list is written by a command and read by a daemon. Holding them together meant
14+
/// `bind` rewrote the whole configuration file to append one string, so a hand-edit
15+
/// made between the CLI reading the file and writing it back was lost.
16+
///
17+
/// Worse, the CLI falls back to a default configuration when the file will not parse.
18+
/// A single malformed character therefore turned the next `bind` into a silent reset of
19+
/// the port and log level, with nothing but a warning on stderr to show for it. A
20+
/// command that shares a USB device has no business rewriting unrelated settings, and
21+
/// now it cannot: this file is the only thing bind and unbind touch.
22+
///
23+
/// The read-modify-write is also serialised. Appending to a list means reading it,
24+
/// adding an entry and writing it back, and two `bind` commands doing that at once
25+
/// would leave only one of the devices bound. Writes are atomic, so a reader never sees
26+
/// a half-written file, but atomicity alone does not prevent a lost update.
27+
public final class BoundDeviceStore {
28+
29+
/// Contents of the state file. An object rather than a bare array so that fields
30+
/// can be added later — a bind timestamp, say — without changing the format.
31+
private struct State: Codable {
32+
var boundDevices: [String]
33+
}
34+
35+
private let path: String
36+
37+
/// Locking uses a file of its own, never the state file.
38+
///
39+
/// Saving replaces the state file by renaming a new one over it, which leaves any
40+
/// lock held on the old file attached to an inode nobody else will open. Two writers
41+
/// could then hold "the lock" on two different files and both proceed. A lock file
42+
/// that is only ever created, never replaced, is the thing they can agree on.
43+
private let lockPath: String
44+
45+
public init(path: String = BoundDeviceStore.defaultPath()) {
46+
self.path = path
47+
self.lockPath = path + ".lock"
48+
}
49+
50+
/// Where the list is kept. Worth reporting in diagnostics, since the answer to
51+
/// "why is this device not shared" is usually in this file.
52+
public var filePath: String { path }
53+
54+
/// `~/.usbipd/bound-devices.json`, beside the configuration but separate from it.
55+
public static func defaultPath() -> String {
56+
let directory = FileManager.default.homeDirectoryForCurrentUser
57+
.appendingPathComponent(ServerConfig.defaultConfigDirName)
58+
return directory.appendingPathComponent("bound-devices.json").path
59+
}
60+
61+
// MARK: - Reading
62+
63+
/// The devices currently bound.
64+
///
65+
/// No lock is taken. Writers replace this file by rename, so a reader sees either
66+
/// the whole previous version or the whole next one, and locking every read would
67+
/// put a system call in the path of every request for no benefit.
68+
public func boundDevices() -> [String] {
69+
guard let data = FileManager.default.contents(atPath: path) else {
70+
return migrateFromLegacyConfigIfNeeded()
71+
}
72+
guard let state = try? JSONDecoder().decode(State.self, from: data) else {
73+
return []
74+
}
75+
return state.boundDevices
76+
}
77+
78+
public func isBound(_ busid: String) -> Bool {
79+
return boundDevices().contains(busid)
80+
}
81+
82+
/// When the file last changed, for callers that cache the list.
83+
public func modificationDate() -> Date? {
84+
return try? FileManager.default.attributesOfItem(atPath: path)[.modificationDate] as? Date
85+
}
86+
87+
// MARK: - Writing
88+
89+
/// Record a device as bound. Returns false if it already was.
90+
@discardableResult
91+
public func bind(_ busid: String) throws -> Bool {
92+
return try mutate { devices in
93+
guard !devices.contains(busid) else { return false }
94+
devices.append(busid)
95+
return true
96+
}
97+
}
98+
99+
/// Forget a device. Returns false if it was not bound.
100+
@discardableResult
101+
public func unbind(_ busid: String) throws -> Bool {
102+
return try mutate { devices in
103+
guard let index = devices.firstIndex(of: busid) else { return false }
104+
devices.remove(at: index)
105+
return true
106+
}
107+
}
108+
109+
/// Read, change and write back while holding an exclusive lock, so that two
110+
/// commands running at once cannot each write a list that omits the other's device.
111+
private func mutate(_ change: (inout [String]) -> Bool) throws -> Bool {
112+
try createDirectoryIfNeeded()
113+
114+
let lock = try FileLock(path: lockPath)
115+
defer { lock.unlock() }
116+
lock.lock()
117+
118+
var devices = boundDevices()
119+
guard change(&devices) else { return false }
120+
try write(devices)
121+
return true
122+
}
123+
124+
private func write(_ devices: [String]) throws {
125+
let encoder = JSONEncoder()
126+
encoder.outputFormatting = [.prettyPrinted, .sortedKeys]
127+
let data = try encoder.encode(State(boundDevices: devices))
128+
// Atomic, so a daemon reading concurrently never sees a partial list.
129+
try data.write(to: URL(fileURLWithPath: path), options: .atomic)
130+
}
131+
132+
private func createDirectoryIfNeeded() throws {
133+
let directory = URL(fileURLWithPath: path).deletingLastPathComponent()
134+
try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
135+
}
136+
137+
// MARK: - Migration
138+
139+
/// Adopt the list from a configuration file written before the two were separated.
140+
///
141+
/// The old key is read but not removed. Rewriting the configuration file is the
142+
/// behaviour this change exists to stop, and a stale `allowedDevices` sitting in it
143+
/// is inert — nothing reads it any more.
144+
private func migrateFromLegacyConfigIfNeeded() -> [String] {
145+
let configPath = ServerConfig.defaultConfigPath()
146+
guard let data = FileManager.default.contents(atPath: configPath),
147+
let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any],
148+
let legacy = object["allowedDevices"] as? [String],
149+
!legacy.isEmpty else {
150+
return []
151+
}
152+
153+
// Best effort. Failing to write the new file simply means trying again next
154+
// time; the devices are still reported as bound either way.
155+
try? createDirectoryIfNeeded()
156+
try? write(legacy)
157+
return legacy
158+
}
159+
}
160+
161+
/// An advisory lock on a file, held for as long as the object is.
162+
private final class FileLock {
163+
private let descriptor: Int32
164+
165+
init(path: String) throws {
166+
descriptor = open(path, O_CREAT | O_RDWR, 0o644)
167+
guard descriptor >= 0 else {
168+
throw ServerError.configurationError(
169+
"Could not open the lock file at \(path): \(String(cString: strerror(errno)))")
170+
}
171+
}
172+
173+
func lock() {
174+
// Blocks until the other holder is done. The critical section is a small read
175+
// and write, so waiting is measured in microseconds.
176+
while flock(descriptor, LOCK_EX) != 0 && errno == EINTR { continue }
177+
}
178+
179+
func unlock() {
180+
flock(descriptor, LOCK_UN)
181+
close(descriptor)
182+
}
183+
}

0 commit comments

Comments
 (0)