Skip to content

Apply a few improvements and fixes#87

Merged
rgantois merged 9 commits into
mainfrom
dev/tprrt/apply-some-fixes
Apr 24, 2026
Merged

Apply a few improvements and fixes#87
rgantois merged 9 commits into
mainfrom
dev/tprrt/apply-some-fixes

Conversation

@tprrt
Copy link
Copy Markdown
Contributor

@tprrt tprrt commented Apr 15, 2026 •
edited
Loading

  • Removed the dead is_in assignment that duplicated is_bulk
  • Replace the unbounded while True loop in _process_dcd_check_data() with a time-based timeout and a small sleep between reads.
  • Wrap copy in try/finally to guarantee mapfileb is closed; also close the PGP-signed handle before reassigning
  • Add if match is None: raise ValueError(...) guards before all three .groups() calls
  • Open a single /dev/null handle shared by both stdout and stderr (avoids two leaked fds)
  • Check raw before slicing; added missing f prefix on the error message string
  • Add an explicit allowed_cmds set; unknown commands are rejected before getattr is called
  • Add a few Github Action workflow improvements

Copilot AI review requested due to automatic review settings April 15, 2026 06:13
@tprrt tprrt force-pushed the dev/tprrt/apply-some-fixes branch from 7e0836c to fd37889 Compare April 15, 2026 06:16
@tprrt tprrt changed the title Apply some improvements and fixes Apply a few improvements and fixes Apr 15, 2026
Copilot AI reviewed Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens robustness and safety across several USB recovery/flash protocols and improves CI workflow reliability by preventing hangs, guarding parsing, and reducing resource leaks.

Changes:

  • Bound the i.MX SDP DCD “check” loop and raise a timeout instead of potentially hanging forever.
  • Harden I/O and parsing paths (HID reads, bmap handling/cleanup, log parsing guards) to avoid crashes and FD leaks.
  • Restrict fastboot CLI dispatch to an explicit allowlist and refine GitHub Actions workflow ordering/triggers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/snagrecover/protocols/imx_sdp.py Replace infinite DCD check loop with bounded retries + timeout error.
src/snagrecover/protocols/hid.py Avoid slicing before validating HID read result; fix error message formatting.
src/snagrecover/protocols/fel.py Remove dead/duplicated endpoint direction assignment.
src/snagrecover/protocols/fastboot.py Remove dead/duplicated endpoint direction assignment.
src/snagflash/ums.py Ensure bmap-related files are reliably closed via try/finally; handle clearsigned bmap.
src/snagflash/fastboot.py Add explicit allowlist for fastboot commands before getattr dispatch.
src/snagfactory/session.py Add match is None guards before .groups() when parsing logs.
src/snagfactory/board.py Share a single /dev/null handle for stdout/stderr redirection.
.github/workflows/main.yml Limit push triggers to main, make tests depend on lint, and adjust install step.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/snagfactory/board.py Outdated
Comment thread src/snagflash/fastboot.py
Comment thread src/snagrecover/protocols/imx_sdp.py Outdated
tprrt added 8 commits April 15, 2026 11:27
@tprrt
protocols: remove dead is_in assignment in USB endpoint detection
ad31021 
The first is_in assignment was identical to is_bulk (checking
bmAttributes instead of bEndpointAddress), making it dead code
immediately overwritten by the correct check on the next line.
Remove the redundant assignment.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
imx_sdp: use time-based timeout in DCD check poll loop
d409207 
Replace the unbounded while True loop in _process_dcd_check_data()
with a time-based timeout and a small sleep between reads.
This ensures the timeout reflects real elapsed time rather than USB
transaction count, and gives hardware enough time to satisfy the
condition before the loop exits.
Then raise TimeoutError if the condition is never met, preventing a
hang when the device is unresponsive.
@tprrt
ums: fix mapfileb file handle leak in bmap_copy()
d2ea8cf 
Wrap the copy operation in try/finally to guarantee mapfileb is
closed even if an exception is raised.

Also explicitly close and clear mapfileb when the bmap file is
PGP-signed, before reassigning the variable, to avoid leaking the
first handle.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
snagfactory: guard regex match results before accessing groups()
7ba2df2 
pattern.match() returns None when the log line doesn't match the
expected format, so calling .groups() on None raises AttributeError.

Add explicit None checks for the summary, results, and board log
sections that raise a descriptive ValueError.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
snagfactory: avoid leaking two file descriptors for /dev/null
316d1e0 
Opening os.devnull twice produced two unclosed file descriptors.
Use a single handle shared by both sys.stdout and sys.stderr.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
hid: fix slice-before-check and missing f-string in libusb_read()
1c37589 
pyusb can return an integer on error. Slicing an int with [1:]
raises TypeError before the guard check was ever reached. Store
the raw result first, validate it, then slice.

Also add the missing f prefix on the HIDError message so the
{length + 1} placeholder is evaluated rather than printed literally.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
snagflash: restrict fastboot dispatch to an explicit command allowlist
a824d01 
getattr() was called with a user-supplied command name after only a
dash-to-underscore substitution, allowing any public method on the
Fastboot object to be invoked. Introduce an explicit set of allowed
command names and reject unknown ones before dispatching.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt
ci: scope trigger, add lint gate, drop redundant ruff install
53ad3a9 
- Restrict push trigger to main branch to avoid duplicate runs on
  feature branch pushes when a PR is already open
- Add needs, lint to test job so failing lint aborts the matrix early
- Remove redundant ruff install from test job (only used in lint job)

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt tprrt force-pushed the dev/tprrt/apply-some-fixes branch 3 times, most recently from 86c2f1f to c15d2a3 Compare April 15, 2026 09:57
@tprrt
ci: mark Python 3.14 jobs as experimental
cf2c4cf 
Python 3.14 tests currently fail because Kivy isn’t pre‑built for that
version.
Apply continue‑on‑error to the 3.14 matrix entries so the failures are
reported but don’t block the overall workflow.

Signed-off-by: Thomas Perrot <thomas.perrot@bootlin.com>
@tprrt tprrt force-pushed the dev/tprrt/apply-some-fixes branch from c15d2a3 to cf2c4cf Compare April 15, 2026 10:05
@rgantois
Copy link
Copy Markdown
Collaborator

rgantois commented Apr 24, 2026

Thanks a lot!

@rgantois rgantois merged commit 1887960 into main Apr 24, 2026
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tprrt @rgantois