upgrade: move cameras on OpenIPC's own bootloader, in the memory they have

[openipc-ai]

Follow-up to #247. That PR was tested against a vendor (XiongMai) bootloader, whose environment is fully literal. A camera on OpenIPC's own bootloader would have been refused, and on a lite image the move would not have fitted in memory anyway.

OpenIPC's bootloader environment

• Kernel read. bootcmd reads the kernel to a variable: sf read ${baseaddr} 0x50000 0x200000. The address may now be any word; only the offset and size are edited.
• Partitions. Its default bootargs say mtdparts=${mtdparts}. That word is left alone, and the mtdparts variable is set instead, with its mtd id read from the variable. A literal mtdparts= is still edited in place, and the variable, if present, follows either way.
• Second kernel read. bootcmdnor, which setnor8m/setnor16m and failsafe run, gets the same edit. A bundle may not set bootcmdnor or mtdparts; upgrade_bundle.py refuses them as -e too.

Memory

Each image used to be copied into a buffer the size of its partition. For an ultimate bundle that is 13 MB, on top of the bundle itself, which already sits in RAM when it is in /tmp. A lite camera booted with mem=32M has about 16 MB available.

Images are now written straight from the mapped bundle, and the end of each one is padded in a single erase-block buffer. The settings partition likewise keeps only the carried image, not 2.7 MB of 0xFF.

Tested on QEMU (hi3516ev300)

OpenIPC's bootloader: the openipc.org 16 MB full image, plus a saved environment in the shape OpenIPC keeps (mtdparts=${mtdparts}, sf read ${baseaddr}):

• generic lite, from the 16 MB layout to the 8 MB one;
• generic ultimate, back from 8 MB to 16 MB;
• the carried settings survived both moves, and mtdparts, bootcmd and bootcmdnor followed each time.

XiongMai bootloader (the #247 case), re-run with this change:

• the 85H50AI flash dump moved from lite to ultimate;
• every setting carried except the two rewritten on each boot;
• kernel and rootfs are byte-identical to their images, and the rest of both partitions is 0xFF.

upgrade_env_test covers the variable load address, mtdparts=${mtdparts}, and reading the mtd id from the variable.

[openipc-ai]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. A long failsafe command is truncated ✓ Resolved

Description

upgrade_bootcmd() writes into new_bootcmdnor with snprintf() even when the output does not
fit, and do_upgrade() later treats a nonempty result as a successful rewrite. If an existing
bootcmdnor exceeds the 1024-byte output buffer, the warning is followed by persistence of its
truncated command rather than preservation of the original.

Code

src/backup.c[R1709-1712]

+        if (nor && !upgrade_bootcmd(nor, mtdwrite[kernel_part].off_flashb,
+                                    mtdwrite[kernel_part].size, new_bootcmdnor,
+                                    sizeof(new_bootcmdnor)))
+            printf("bootcmdnor has no kernel read to edit; left as it is\n");

Evidence

The formatter returns false on truncation after writing a prefix into the buffer; the new caller
warns but later uses the buffer's first byte to decide whether to persist it. U-Boot environment
values can exceed this local buffer.

src/upgrade_env.c[104-109]
src/backup.c[1641-1644]
src/backup.c[1708-1718]
src/backup.c[1771-1774]
src/uboot.c[109-122]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed secondary-command rewrite can leave nonempty truncated output that is subsequently saved.
## Fix Focus Areas
- src/backup.c[1708-1718]
- src/backup.c[1771-1774]
## Recommended Fix
Record the boolean result of `upgrade_bootcmd()` and gate both display and persistence on that result, not on whether the output buffer is nonempty. Refuse an unsafe layout move when an existing kernel-read command cannot fit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wiping settings invokes undefined behavior ✓ Resolved

Description

do_flash() computes mtdbackup[i].data + c * chunk before checking whether a padded block has any
payload. With --wipe-settings, the new settings entry has a positive size but a null data pointer
and zero payload, so every settings block evaluates pointer arithmetic on null before selecting the
erase-block buffer.

Code

src/backup.c[R956-959]

+                    const char *src = mtdbackup[i].data + c * chunk;
+                    if (mtdbackup[i].padded &&
+                        (c + 1) * chunk > mtdbackup[i].payload) {
+                        size_t have = c * chunk < mtdbackup[i].payload

Evidence

The wiped settings entry is initialized without a data pointer, yet flashing computes a pointer from
it before the padded branch replaces that pointer with the erased buffer.

src/backup.c[956-968]
src/backup.c[1658-1684]
src/backup.c[1753-1759]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An empty padded settings entry forms a pointer from null during flashing.
## Fix Focus Areas
- src/backup.c[956-968]
- src/backup.c[1658-1662]
## Recommended Fix
Choose the padding buffer before computing a source pointer for a block with no payload. Only form `data + offset` when payload bytes actually exist in that block.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Partitions edited in an unused variable ✓ Resolved

Description

upgrade_bootargs_mtdparts_is_var() accepts any mtdparts= value beginning with $, but
do_upgrade reads and rewrites only the variable literally named mtdparts while leaving the
bootargs reference unchanged. If bootargs names another variable, such as ${mtdpartsnor}, and a
separate mtdparts variable exists, validation succeeds and root= advances to the new rootfs
index while the kernel still receives the old partition table.

Code

src/backup.c[R1493-1496]

+        if (!upgrade_mtd_id(ba, id, sizeof(id)) &&
+            !(upgrade_bootargs_mtdparts_is_var(ba) &&
+              upgrade_mtd_id_value(uboot_env_get_param("mtdparts"), id,
+                                   sizeof(id)))) {

Evidence

upgrade_bootargs_mtdparts_is_var() checks only for $ after mtdparts=
(src/upgrade_env.c[29-32]). The keep_front check and final environment update use the fixed name
mtdparts (src/backup.c[1493-1496], src/backup.c[1771-1772]), while variable-mode bootargs
rewriting retains the original reference (src/upgrade_env.c[69]); therefore, a different variable
named in bootargs is neither checked nor updated.

src/upgrade_env.c[29-32]
src/backup.c[1492-1504]
src/backup.c[1768-1774]
src/upgrade_env.c[47-75]
src/upgrade_env.c[50-52]
src/upgrade_env.c[69-72]
src/backup.c[1492-1496]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`upgrade_bootargs_mtdparts_is_var()` treats any dollar-prefixed `mtdparts=` value as a reference to the variable the upgrade updates. When bootargs names a different variable, validation can succeed even though that variable retains the old partition table.
## Fix Focus Areas
- src/upgrade_env.c[29-32]
- src/backup.c[1492-1504]
- src/backup.c[1769-1772]
## Recommended Fix
Either accept only the exact references `mtdparts=${mtdparts}` and `mtdparts=$mtdparts`, followed by a space or NUL, and reject other references before anything is stopped; or resolve the referenced variable name and use it consistently for validation and the environment get/set operations. Add a test where bootargs names a different variable while `mtdparts` also exists.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (1)

4. Failsafe boot can read the old kernel ✓ Resolved

Description

do_upgrade() treats a failed bootcmdnor rewrite as a warning and continues the layout migration
with the original command unchanged. When bootcmdnor contains multiple sf read operations or a
read that cannot be parsed, the upgrade moves the kernel and updates the primary boot command, but
the setnor8m/setnor16m or failsafe path still reads from the old offset and size.

Code

src/backup.c[R1708-1712]

+        const char *nor = uboot_env_get_param("bootcmdnor");
+        if (nor && !upgrade_bootcmd(nor, mtdwrite[kernel_part].off_flashb,
+                                    mtdwrite[kernel_part].size, new_bootcmdnor,
+                                    sizeof(new_bootcmdnor)))
+            printf("bootcmdnor has no kernel read to edit; left as it is\n");

Evidence

upgrade_bootcmd returns false when it finds no read, more than one read, or a read it cannot parse
(upgrade_env.c lines 87–99). The caller in src/backup.c lines 1706–1712 only prints a message on
failure and continues; the upgrade then updates the primary boot path while bootcmdnor, which the
repository identifies as the failsafe command, retains its old kernel read.

src/backup.c[1706-1718]
src/upgrade_env.c[80-100]
src/upgrade_env.c[82-88]
src/upgrade_env_test.c[85-88]
src/backup.c[1693-1712]
src/backup.c[1768-1776]
README.md[309-314]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A `bootcmdnor` containing a kernel read that cannot be rewritten can remain pointed at the old layout while the upgrade proceeds.
## Fix Focus Areas
- src/backup.c[1706-1712]
- src/backup.c[1768-1774]
## Recommended Fix
Skip `bootcmdnor` only when it contains no `sf read`. If it contains a read and `upgrade_bootcmd` fails, print an error, set `ret = 1`, and `goto bailout` before isolation or flashing, as is done for `bootcmd`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗