simplify swupdate install to fix partition switching on 3.22+ #157
Conversation
WalkthroughReplaced iterative swupdate invocation with a single direct Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codexctl/device.py (2)
694-702: Consider logging stdout for consistency and debugging.The remote execution path doesn't log stdout after successful completion, while the local path does (lines 755-757). For consistency and debugging purposes, consider capturing and logging the stdout.
Apply this diff to add stdout logging:
command = f"/usr/sbin/swupdate-from-image-file {out_location}" self.logger.debug(command) _stdin, stdout, _stderr = self.client.exec_command(command) exit_status = stdout.channel.recv_exit_status() if exit_status != 0: print("".join(_stderr.readlines())) raise SystemError("Update failed!") + +self.logger.debug( + f"Stdout of swupdate: {''.join(stdout.readlines())}" +)
743-750: Consider avoiding shell=True for better security posture.While the security risk is mitigated since this runs locally with controlled input, using
shell=Trueis flagged by static analysis. Consider using the array form of subprocess invocation for better security hygiene.Apply this diff to use array form:
-with subprocess.Popen( - command, - text=True, - shell=True, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - env={"PATH": "/bin:/usr/bin:/sbin:/usr/sbin"}, -) as process: +with subprocess.Popen( + ["/usr/sbin/swupdate-from-image-file", version_file], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env={"PATH": "/bin:/usr/bin:/sbin:/usr/sbin"}, +) as process:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
codexctl/device.py
702-702: Avoid specifying long messages outside the exception class
(TRY003)
743-743: subprocess call with shell=True identified, security issue
(S602)
753-753: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for windows-latest
- GitHub Check: Build for macos-latest
- GitHub Check: Build for remarkable
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
codexctl/device.py (3)
694-702: Quote remote image path and close SFTP client for robustness
out_locationis interpolated directly into a shell command string, and the SFTP client is never closed. Even thoughout_locationis derived fromos.path.basename, it can still contain spaces or special characters, and leaving SFTP sessions open is avoidable.Consider:
- Using a context manager for SFTP so it always closes.
- Shell-quoting
out_locationto handle spaces/special chars safely.@@ - if self.client: - ftp_client = self.client.open_sftp() + if self.client: + with self.client.open_sftp() as ftp_client: @@ - command = f"/usr/sbin/swupdate-from-image-file {out_location}" + import shlex # add near the top of the file + command = f"/usr/sbin/swupdate-from-image-file {shlex.quote(out_location)}" self.logger.debug(command) _stdin, stdout, _stderr = self.client.exec_command(command) @@ - if exit_status != 0: + if exit_status != 0: print("".join(_stderr.readlines())) raise SystemError("Update failed!")
739-754: Preserve original error context and avoid clobbering environmentThe local
subprocess.check_outputcall looks good and avoidsshell=True, but two small tweaks would improve debuggability and compatibility:
- Chain the
SystemErrorfrom the originalCalledProcessErrorso callers can inspect the root cause.- Merge your custom
PATHwith the existing environment instead of replacing it entirely, in caseswupdate-from-image-filerelies on other vars.@@ - try: - output = subprocess.check_output( - command, - stderr=subprocess.STDOUT, - text=True, - env={"PATH": "/bin:/usr/bin:/sbin:/usr/sbin"}, - ) + try: + env = os.environ.copy() + env["PATH"] = "/bin:/usr/bin:/sbin:/usr/sbin" + output = subprocess.check_output( + command, + stderr=subprocess.STDOUT, + text=True, + env=env, + ) self.logger.debug(f"Stdout of swupdate: {output}") except subprocess.CalledProcessError as e: print(e.output) - raise SystemError("Update failed") + raise SystemError("Update failed") from e
670-681: Align docstring with actual exception typeThe docstring says this method raises
SystemExiton failure, but the implementation now raisesSystemError. That mismatch can surprise callers and any downstream tooling relying on the documented contract.Either update the docstring to mention
SystemErroror change the raised exception type to match the documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
codexctl/device.py
702-702: Avoid specifying long messages outside the exception class
(TRY003)
744-744: subprocess call: check for execution of untrusted input
(S603)
753-753: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
753-753: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build for windows-latest
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for macos-latest
- GitHub Check: Build for remarkable
2 participants
In testing 3.22+ support, I saw a couple of times when the partition didn't switch after install, but I couldn't reliably replicate it. I believe I've since figured it out: in 3.22+, reMarkable changed how partition switching works:
/sys/devices/platform/lpgpr/swu_statusto indicate success/failurerm-apply-otachecks if swu_status == 1 and callsrootdev --switchto change the boot partitionThe previous implementation ran swupdate twice (once with -e stable,copy1 and once with -e stable,copy2) to brute-force which partition to install to. One would succeed, one would fail with "over our current root". A code comment calls this a "terrible hack".
This PR changes to using
/usr/sbin/swupdate-from-image-filescript instead of manually invokingswupdate. This script:/usr/lib/swupdate/conf.d/09-swupdate-argsswupdate -gswupdateonly onceI've verified that both
/usr/sbin/swupdate-from-image-fileand/usr/lib/swupdate/conf.d/09-swupdate-argsexist in the earliest swu firmware (3.11.3.3) and the latest (3.24.0.149), so this should be a reliable mechanism.I've successfully tested a
codexctl installof 3.24.0.149 to both A and B partitions of my rMPP and confirmed the partition switch happened successfully both times.In addition to resolving this issue, this change simplifies and cleans up this section of the codebase.
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.