[RFC] Armbianmonitor refactor#9327
Open
EvilOlaf wants to merge 11 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new armbian-debug utility to collect, sanitize, and upload comprehensive Armbian diagnostic data (with interactive stdout fallback) and removes/deprecates upload-related collector functionality from armbianmonitor, redirecting support-info upload flows to armbian-debug. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Script as armbian-debug
participant Shell as ShellCmds
participant Curl as curl
participant Paste as PasteServer
User->>Script: run armbian-debug (interactive)
Script->>User: 5s prompt to switch to stdout
Script->>Shell: CollectSupportInfo() (dmesg, hw, lshw, iostat, etc.)
Shell-->>Script: diagnostic payload
Script->>Script: sanitize payload (redact IPs)
Script->>Curl: upload to PasteServer A
Curl-->>Script: exit status (PIPESTATUS)
alt upload success
Paste-->>Script: URL
Script->>User: success message with URL
else upload failure
Script->>Curl: try next PasteServer...
alt all attempts fail
Script->>User: fallback -> print numbered, redacted stdout
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/bsp/common/usr/bin/armbian-debug`:
- Around line 57-106: The curl call inside the paste_server upload loop can hang
indefinitely; update the curl invocation in the for paste_server loop (the
pipeline that ends with curl -s --fail --data-binary `@-`
"https://${paste_server}/log") to include connection and overall timeouts (e.g.
--connect-timeout 10 and --max-time 30) so stalled servers fail fast and allow
the script to fall back to stdout; keep the rest of the pipeline and PIPESTATUS
handling unchanged.
Member
Author
|
I have no idea where this conflict comes from and how I produced that but nevertheless this is more POC and RFC, so.... |
igorpecovnik
added
05
Member
|
I have very strict orders from my psychiatrist to never look at this |
Member
Author
|
Haha. |
Member
|
What is the status here? |
Member
Author
|
Waiting for review :) |
Member
Member
Author
|
Ah. Yes, I did not do any tests yet since it was more like a request for feedback if I went too far or not far enough. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bsp/common/usr/bin/armbianmonitor (1)
128-128:⚠️ Potential issue | 🟡 MinorTypo: "referance" → "reference".
- # Kept for referance. + # Kept for reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbianmonitor` at line 128, Fix the typo in the retained comment: change the string "# Kept for referance." to "# Kept for reference." in the armbianmonitor script (locate the comment line containing "Kept for referance."). Ensure only the spelling is corrected and no other text or behavior is modified.
🧹 Nitpick comments (2)
packages/bsp/common/usr/bin/armbianmonitor (2)
718-719: Subshell()meansroutemay still be missing after install failure.The
(apt-get update && apt-get -f -y install net-tools)runs in a subshell. If installation fails, the script continues androutecalls on line 722+ will fail. Consider using{ ...; }or adding a post-check. That said, this pattern is used elsewhere in the file (lines 518-525), so it's consistent with existing style.Suggested improvement
- which route > /dev/null 2>&1 || (apt-get update && apt-get -f -y install net-tools) + if ! command -v route > /dev/null 2>&1; then + apt-get update && apt-get -f -y install net-tools + command -v route > /dev/null 2>&1 || { echo "Error: 'route' command not available. Install net-tools." >&2; exit 1; } + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbianmonitor` around lines 718 - 719, The check that ensures net-tools is installed uses a subshell for the install step so a failed install won't stop the main shell and subsequent `which route` usage may still fail; change the construct that follows `which route` (the `(apt-get update && apt-get -f -y install net-tools)` invocation) to run in the current shell (e.g., use a brace group or sequential commands) and/or add an explicit post-install verification of `route` success, ensuring the `apt-get` failure propagates or the script exits if `route` is still missing; locate the `which route` check and the net-tools install call to apply this fix.
58-58: Remove unusedr,R,p,Pflags from getopts string.These options are present in the getopts string but have no corresponding case handlers, so they fall through to the default
*case and print "Invalid flag". Since they're unused dead code, remove them to match the active handlers.- while getopts 'hHuUrRmMsnNd:c:C:pPv' c; do + while getopts 'hHuUmMsnNd:c:C:v' c; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbianmonitor` at line 58, The getopts option string in the while loop (the "while getopts 'hHuUrRmMsnNd:c:C:pPv' c; do" line) contains unused flags r, R, p, and P; remove those characters from the quoted option string so it only lists the active flags handled in the case block (e.g., change the string to omit rR pP while keeping the other options and any arguments like : where needed), ensuring the getopts invocation and its variable c remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bsp/common/usr/bin/armbianmonitor`:
- Around line 83-100: The "Launching armbian-debug now..." message is printed
before verifying the tool exists in the u|U option handler; move the message so
it is only printed after a successful existence check. Modify the u|U case
around the checks of /usr/bin/armbian-debug and "$(dirname "$0")/armbian-debug"
(the exec branches) so you perform the -x existence tests first, then print
"Launching armbian-debug now..." right before calling exec, and leave the
existing error echo/exit path unchanged if neither path is found.
---
Outside diff comments:
In `@packages/bsp/common/usr/bin/armbianmonitor`:
- Line 128: Fix the typo in the retained comment: change the string "# Kept for
referance." to "# Kept for reference." in the armbianmonitor script (locate the
comment line containing "Kept for referance."). Ensure only the spelling is
corrected and no other text or behavior is modified.
---
Nitpick comments:
In `@packages/bsp/common/usr/bin/armbianmonitor`:
- Around line 718-719: The check that ensures net-tools is installed uses a
subshell for the install step so a failed install won't stop the main shell and
subsequent `which route` usage may still fail; change the construct that follows
`which route` (the `(apt-get update && apt-get -f -y install net-tools)`
invocation) to run in the current shell (e.g., use a brace group or sequential
commands) and/or add an explicit post-install verification of `route` success,
ensuring the `apt-get` failure propagates or the script exits if `route` is
still missing; locate the `which route` check and the net-tools install call to
apply this fix.
- Line 58: The getopts option string in the while loop (the "while getopts
'hHuUrRmMsnNd:c:C:pPv' c; do" line) contains unused flags r, R, p, and P; remove
those characters from the quoted option string so it only lists the active flags
handled in the case block (e.g., change the string to omit rR pP while keeping
the other options and any arguments like : where needed), ensuring the getopts
invocation and its variable c remain unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/bsp/common/usr/bin/armbian-debug (4)
28-37: Auto-installing curl viaapt-getwithoutapt-get updatemay fail on a stale cache.If the package lists are stale or missing,
apt-get -f -y install curlcould fail. Consider runningapt-get updatefirst, or at minimum warn the user that installation failed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbian-debug` around lines 28 - 37, The script's curl-install branch (the if checking "command -v curl") runs apt-get -f -y install curl which can fail on stale package lists; modify that branch to run apt-get update first (or run apt-get update && apt-get -f -y install curl), and on non-root or if install fails print a clear error advising the user to run apt-get update and retry (or manually install curl). Ensure you update the block that contains the command -v curl check and the apt-get -f -y install curl invocation to include the update and a failure message.
109-119:whichis deprecated; usecommand -vconsistently.Line 115 uses
which lshwwhile elsewhere in this file (line 29) you correctly usecommand -v.whichis not POSIX and may not be available on minimal installs.Proposed fix
- which lshw > /dev/null 2>&1 && ( + command -v lshw > /dev/null 2>&1 && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbian-debug` around lines 109 - 119, In CollectSupportInfo() replace the non-POSIX check using "which lshw" with a POSIX-safe "command -v lshw" check to match the rest of the script; update the conditional that currently reads which lshw > /dev/null 2>&1 to use command -v lshw > /dev/null 2>&1 so the lshw block executes only when lshw is present and avoids relying on which.
130-133:nvmewith no arguments may not exit 0 on all versions.Running
nvmewith no subcommand to check availability is fragile — some versions print help and exit 0, others may exit non-zero. Usingcommand -v nvmewould be a more reliable existence check, consistent with the curl check above.Proposed fix
- nvme > /dev/null 2>&1 && ( + command -v nvme > /dev/null 2>&1 && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbian-debug` around lines 130 - 133, The availability check currently runs the binary itself with "nvme > /dev/null 2>&1 && (...)" which is fragile; replace that check with a path lookup via "command -v nvme >/dev/null 2>&1 && (...)" so the script reliably detects presence of the nvme executable before running "nvme list" (update the conditional that surrounds the echo and nvme list invocation).
120-125: Same pattern: usecommand -vinstead of bare command invocation forlsusbandlspci.Lines 120 and 126 run
lsusb/lspciwith no args just to check existence. Prefercommand -vfor consistency and reliability.Proposed fix
- lsusb > /dev/null 2>&1 && ( + command -v lsusb > /dev/null 2>&1 && ( ... - lspci > /dev/null 2>&1 && ( + command -v lspci > /dev/null 2>&1 && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbian-debug` around lines 120 - 125, Replace the bare command existence checks that use `lsusb > /dev/null 2>&1 && (...)` and `lspci > /dev/null 2>&1 && (...)` with POSIX-safe checks using `command -v lsusb >/dev/null 2>&1 && (...)` and `command -v lspci >/dev/null 2>&1 && (...)`; keep the inner blocks (the `echo` calls, `lsusb ${VERBOSE}`, `lsusb -t`, and the corresponding `lspci` usage) unchanged so the script verifies availability via `command -v` before invoking `lsusb`/`lspci`.packages/bsp/common/usr/bin/armbianmonitor (1)
718-719: Subshell swallows installation failure fornet-tools.The
(apt-get update && apt-get ... install net-tools)runs in a subshell due to(...). If installation fails, the script continues silently androutewill fail on line 722 with a confusing error. Consider using{ ...; }instead, or adding an explicit check after install.Proposed fix
- which route > /dev/null 2>&1 || (apt-get update && apt-get -f -y install net-tools) + if ! command -v route > /dev/null 2>&1; then + apt-get update && apt-get -f -y install net-tools || { echo "'route' command not found and installation failed." >&2; exit 1; } + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bsp/common/usr/bin/armbianmonitor` around lines 718 - 719, The current check "which route > /dev/null 2>&1 || (apt-get update && apt-get -f -y install net-tools)" uses a subshell so failures are swallowed; replace the subshell usage with a compound command or explicit post-install check: run apt-get update and apt-get -f -y install net-tools in the current shell (use { ...; } or sequential commands) and then verify the install by re-checking "which route" (or testing exit status) and aborting or logging an error if the install failed; update the block that contains the "which route" check and the install invocation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bsp/common/usr/bin/armbian-debug`:
- Line 73: The dmesg invocation uses the --color=always flag which embeds ANSI
escape sequences into uploaded paste data; update the dmesg call (the line
invoking dmesg) to use --color=never or remove the color flag entirely so output
has no ANSI escapes and won’t pollute paste uploads or interfere with IP
sanitization.
- Around line 56-106: The upload path and both stdout/fallback paths collect
different info and re-run CollectSupportInfo which can produce inconsistent
results; change the flow to collect the full payload once into a temp file
(capture LC_ALL=C date, dmesg --color=always, and CollectSupportInfo into a temp
file), then use that temp file for the curl upload loop (while still applying
the IP-scrubbing sed) and for any stdout/fallback printing (apply awk/nl same as
current). Update logic around paste_servers, upload_success, and PIPESTATUS to
read from the temp file instead of re-invoking CollectSupportInfo, and ensure
the temp file is cleaned up at the end.
---
Nitpick comments:
In `@packages/bsp/common/usr/bin/armbian-debug`:
- Around line 28-37: The script's curl-install branch (the if checking "command
-v curl") runs apt-get -f -y install curl which can fail on stale package lists;
modify that branch to run apt-get update first (or run apt-get update && apt-get
-f -y install curl), and on non-root or if install fails print a clear error
advising the user to run apt-get update and retry (or manually install curl).
Ensure you update the block that contains the command -v curl check and the
apt-get -f -y install curl invocation to include the update and a failure
message.
- Around line 109-119: In CollectSupportInfo() replace the non-POSIX check using
"which lshw" with a POSIX-safe "command -v lshw" check to match the rest of the
script; update the conditional that currently reads which lshw > /dev/null 2>&1
to use command -v lshw > /dev/null 2>&1 so the lshw block executes only when
lshw is present and avoids relying on which.
- Around line 130-133: The availability check currently runs the binary itself
with "nvme > /dev/null 2>&1 && (...)" which is fragile; replace that check with
a path lookup via "command -v nvme >/dev/null 2>&1 && (...)" so the script
reliably detects presence of the nvme executable before running "nvme list"
(update the conditional that surrounds the echo and nvme list invocation).
- Around line 120-125: Replace the bare command existence checks that use `lsusb
> /dev/null 2>&1 && (...)` and `lspci > /dev/null 2>&1 && (...)` with POSIX-safe
checks using `command -v lsusb >/dev/null 2>&1 && (...)` and `command -v lspci
>/dev/null 2>&1 && (...)`; keep the inner blocks (the `echo` calls, `lsusb
${VERBOSE}`, `lsusb -t`, and the corresponding `lspci` usage) unchanged so the
script verifies availability via `command -v` before invoking `lsusb`/`lspci`.
In `@packages/bsp/common/usr/bin/armbianmonitor`:
- Around line 718-719: The current check "which route > /dev/null 2>&1 ||
(apt-get update && apt-get -f -y install net-tools)" uses a subshell so failures
are swallowed; replace the subshell usage with a compound command or explicit
post-install check: run apt-get update and apt-get -f -y install net-tools in
the current shell (use { ...; } or sequential commands) and then verify the
install by re-checking "which route" (or testing exit status) and aborting or
logging an error if the install failed; update the block that contains the
"which route" check and the install invocation accordingly.
Member
|
Is it near completion? |
Member
Author
|
Perhaps I can throw some nitpicks at llm to fix them but besides that I think so. |
Perhaps add a reference to sbcbench later for proper benchmarking
-D was referencing to ix.io which has been dead for years now and nobody complained 7z benchmark was interesting years ago when sbcs weren't as powerful as they are today. Perhaps also here reference to sbcbench for proper results.
well...actually has been there for a while..
this code hasn't been touched by me I believe, so these bugs must have been present for a loooong time...
EvilOlaf
force-pushed
the
armbianmonitor-refactor
branch
from
7a7d2e0 to
5fea993
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
split log upload into dedicated tool
-u/-Uinarmbianmonitorintact but add deprecation noticearmbianmonitor
rpimonitorand dirty binary patch hacksbcbenchlater for proper resultsix.iowhich has been dead for years and nobody ever complained@rpardini you may wanna have a look at this...or not :D
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Refactor