SDWire: record stable device link by id instead of diskseq#340
Conversation
WalkthroughThe changes introduce a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant D as Driver
participant FS as Filesystem
D->>FS: Call effective_storage_device
alt storage_device is None
D->>FS: Retrieve device link from USB subsystem
end
D->>D: Set storage_device attribute
D->>FS: Perform read/write operations
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Can we check if it really works? I remember that sdwire wasn't specially good with "disk by-id" ... as it didn't implement any serial number for the SD card reader. |
From my two minutes of testing it seems to pass-through the id of the inserted sdcard. |
May be it was my card that didn't have an id? Would it work if we keep looking up the device by seq, but do it always before attempting to write or read? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py (1)
34-38: Improve robustness by handling potential missing device linksCurrently, there's no fallback mechanism if no device links matching the criteria are found. Consider adding a logging statement or fallback strategy for cases where SD cards might lack proper identifiers.
for storage_device in filter( lambda link: link.startswith("/dev/disk/by-diskseq/"), block.device_links ): return storage_device +# Fallback to direct block device if no stable links found +return block.device_node
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (5)
packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py (5)
106-107: LGTM - Good centralization of storage device logicCentralizing the effective storage device lookup improves code maintainability and ensures consistent behavior.
115-121: Good improvement to error handlingThe specific handling of error codes (123 for "No medium found" and 5 for "Input/output error") makes the code more robust. This properly distinguishes between expected transient errors and unexpected errors that should be raised.
123-124: Good defensive programming practiceChecking if
fdexists in locals before closing it prevents potential errors in error handling flow.
127-128: Consistent use of storage device across operationsThe consistent pattern of retrieving and using the storage device path across read and write operations ensures reliability. The changes effectively centralize the storage device determination logic.
Also applies to: 132-133, 141-142
24-41:Details
❓ Verification inconclusive
Inconsistency between implementation and PR objective
The PR objective mentions recording stable device links by ID instead of diskseq, but line 36 still filters for paths starting with
/dev/disk/by-diskseq/. Consider changing this to use/dev/disk/by-id/instead to align with the PR's stated goal.- lambda link: link.startswith("/dev/disk/by-diskseq/"), block.device_links + lambda link: link.startswith("/dev/disk/by-id/"), block.device_links
🏁 Script executed:
#!/bin/bash # Check if the device actually has by-id symlinks available for udev in $(udevadm info --export-db | grep -o 'P: .*sd.*' | awk '{print $2}'); do echo "Device: $udev" udevadm info --query=symlink --name=$udev | grep -q "disk/by-id" && echo "Has by-id links" || echo "No by-id links" echo "---" doneLength of output: 168
Action Required: Update Link Filtering to Use
/dev/disk/by-id/It appears that the shell script for verifying the presence of "by-id" symlinks failed because the
udevadmcommand isn't available in our sandbox. As a result, the verification was inconclusive. The code still filters for/dev/disk/by-diskseq/links, whereas the PR objective is to record stable device links using/dev/disk/by-id/.
- File:
packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py- Lines: 24-41
- Required Change:
- lambda link: link.startswith("/dev/disk/by-diskseq/"), block.device_links + lambda link: link.startswith("/dev/disk/by-id/"), block.device_linksPlease update the logic accordingly. Important: Since the automated verification did not yield the expected output, manually verify on a system where
udevadmis available that devices indeed expose stable "by-id" symlinks. This manual check is necessary to ensure the change aligns with the intended behavior.
2 participants
Closes-Issue: #341
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features