sysupgrade: restore what an abort stopped, and mark the camera's return - #2419
Merged
Merged
Conversation
Two loose ends from #2417, both of them a camera left in a state nobody can see from outside. check_sdcard kept everything it had been given. It runs after create_lock and free_resources, and a recovery file on the card exited straight out of it, so the lock stayed in /tmp and every later run answered "Another sysupgrade process is already running!" until a reboot cleared it; syslogd, klogd, ntpd and crond stayed stopped; and majestic stayed gutted by the SIGQUIT free_resources sends. Since #2417 it also left a collector holding "logging stops here until this camera returns" for a camera that was never touched. Nothing is written at that point, so die() already does the right thing -- no-reboot branch, services restarted, lock dropped. Use it. Measured on an hi3516av300 (nightly-20260913-910e724), all three stoplist files on the card, kernel path deliberately nonexistent: pre-fix with this change syslogd/klogd/ntpd/crond all four running all DOWN lock leaked lock cleaned up next run refuses with run proceeds "Another sysupgrade process is already running!" majestic reports "up" in both columns, and that is the trap: pidof finds it because SIGQUIT is majestic's "release the SDK and keep serving" signal. In the pre-fix state a snapshot returns HTTP 200 with 0 bytes and the log says "stop_sdk: Stop sdk Ok!". The camera looks alive and has no video. The second half is the other end of that sentence. free_resources tells a collector the log stops "until this camera returns"; nothing ever said it had. S41bootmsg logs "this camera is up on <BUILD_ID>" once the network can carry it. The build id is the payload: it is the only word from the camera itself that the flash took, or that the bootloader fell back to what it had. Gated on SYSLOG_REMOTE, so a camera that forwards nothing sources a file it does not have and exits -- 610 bytes once comments are stripped. Backgrounded, so the boot pays nothing: the script returns in 0.00s and the waiting happens in a subshell. That matters because it can wait fifteen seconds for a lease, and blocking would put that in front of majestic. What it waits FOR is the part worth reading. A two-second settle after the address -- failsafe-rescue's -- is wrong here: the marker went out at 13:28:10 and the first line the collector received that boot was majestic's at 13:28:11. It missed by one second, and forwarding is UDP with nothing to retry it, so it was gone. It now sends one ICMP packet at the collector first, resolving the ARP entry the datagram needs instead of guessing how long that takes. Two reboots, two deliveries: 2026-09-14T13:33:52+01:00 10.216.128.39 warning boot this camera is up on nightly-20260913-910e724 2026-09-14T13:34:49+01:00 10.216.128.39 warning boot this camera is up on nightly-20260913-910e724 test_sysupgrade.sh gains four behaviour checks and four drift checks, and the mount/umount stubs are now stateful so check_sdcard's unmount loop can be driven at all. The two assertions that encode the abort fix were confirmed to fail against the pre-fix script; the other two stay green, so they are pinning the fix and not the weather.
widgetii
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
September 14, 2026 12:38
PR Summary by QodoClean up aborted sysupgrades and announce camera return
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Review found the half I had not tested. Every measurement behind the marker was taken against a literal IP, and a name behaves differently: busybox resolves the destination once, when syslogd starts -- S01, before the network -- and retries only every 120 s. etc/default/syslogd has said so since #2415 and tells people to prefer an IP for exactly this reason. So on a name everything sent in the first two minutes is dropped however ready the path is, and this marker, at roughly eighteen seconds, was precisely the line that silently never arrived. Reproduced on an hi3516av300 pointed at a hostname collector. Local: Sep 14 12:49:55 hi3516av300-imx415 user.warn boot: this camera is up on nightly-20260913-910e724 Collector, same boot: nothing -- while 48 other lines from that window did get through, once the daemon had re-resolved. The marker now waits the window out on a name. Late and delivered beats punctual and dropped, and the build id is no less true two minutes on. It is free: the wait was already in a subshell, so nothing on the boot path is waiting for it. Same board, same hostname collector, after: Sep 14 12:55:39 hi3516av300-imx415 user.warn boot: this camera is up on nightly-20260913-910e724 2026-09-14T13:55:39.271358+01:00 10.216.128.39 warning boot this camera is up on nightly-20260913-910e724 Deliberately not the other repair, which is to restart syslogd so it resolves again. The buffer logread and the WebUI read is in RAM and dies with the daemon, so that trades the whole local record of the boot -- the thing that still exists when forwarding does not -- for one forwarded line. Two drift checks now pin both halves of that: that the wait exists, and that it is not done by restarting syslogd.
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
1 participant
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.
Problem
Two loose ends from #2417, both of them a camera left in a state nobody can see from outside.
check_sdcardkept everything it had been given. It runs aftercreate_lockandfree_resources, so a recovery file on the card exited straight out of it: the lock stayed in/tmpand every later run refused with "Another sysupgrade process is already running!" until a reboot cleared it; syslogd, klogd, ntpd and crond stayed stopped; and majestic stayed gutted by the SIGQUITfree_resourcessends. Since #2417 it also left a collector holding "logging stops here until this camera returns" for a camera that was never touched. Nothing is written at that point, sodie()already does the right thing — no-reboot branch, services restarted, lock dropped.Nothing ever said the camera came back.
free_resourcestells a collector the log stops "until this camera returns"; the return was only implicit, whenever the camera next happened to say something. A reader could not tell a camera that came back in ninety seconds from one that is still gone, and had nothing saying which image it came back on.Affects every board — both files are in the shared overlay (
ci-matrix: 99/99).Hardware tested on
hi3516av300 (
hi3516av300-imx415, imx415, nightly-20260913-910e724), forwarding to a remote rsyslog collector. Both halves exercised on that board: the abort path with all three stoplist files on the card, and the boot marker across two reboots.Evidence
1. The abort. All three stoplist files on the card, kernel path deliberately nonexistent so nothing could be written even if the check were skipped.
Before:
majestic: upis the trap —pidoffinds it because SIGQUIT is majestic's "release the SDK and keep serving" signal. In that state it has no video:After:
That run also confirmed #2417's retraction on hardware for the first time — it had only test coverage until now. Both halves reached the collector:
2. The boot marker. The first version used the two-second settle
failsafe-rescueuses, and that is wrong here — it went out one second too early and was dropped, because forwarding is UDP and nothing retries it:Before:
It now sends one ICMP packet at the collector first, which resolves the ARP entry the datagram needs instead of guessing how long that takes. Two reboots, two deliveries:
After:
Costs the boot nothing —
startreturns in0.00sin every case, the waiting is in a subshell, and with forwarding off no subshell is spawned at all:610 bytes on the image once comments are stripped (3998 raw).
3. Tests.
test_sysupgrade.shgains four behaviour checks and four drift checks; themount/umountstubs are now stateful socheck_sdcard's unmount loop can be driven at all. The two assertions that encode the abort fix were confirmed to fail against the pre-fix script, and the two unrelated ones stayed green:Full local gate set green:
test_sysupgrade,test_shell_parse(STRICT, 145 scripts incl. the new one),test_strip_shell_comments(STRICT),test_load_hisilicon,test_check_mac,test_automount,ci-matrix --self-test.Scope
general/package/all-patches/linux/(those go to OpenIPC/linux)general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my boardLD_PRELOAD, and no binaries that cannot be rebuilt from source