Skip to content

backup: stream UBI volumes, back up all of Novatek's partitions (#234) - #235

Merged
openipc-ai merged 4 commits into
masterfrom
backup-streams-ubi-volumes
Oct 9, 2026
Merged

openipc-ai merged 4 commits into
masterfrom
backup-streams-ubi-volumes

Conversation

@openipc-ai

@openipc-ai openipc-ai commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #234.

backup: stream UBI volumes instead of holding them in RAM

ipctool upload with a backup on a Novatek NT98566 with 128 MB of NAND
refused with "Not sent: 1 partition(s) of the flash could not be read".
Its rootfs is a 62.5 MB UBI volume, and the backup malloc()ed every UBI
volume whole and held them all at once -- more than the camera has
free. The sha1s in the same report were fine, because those stream.

  • A UBI volume is now a span with a path: http_post() and save_file()
    read it in 64 KB pieces while sending or writing. Nothing else
    changes in the body or the file. A file that ends short of its length
    stops the upload with ERR_SOURCE, since the Content-Length is already
    sent.
  • A partition that cannot be read is named, with the reason
    ("rootfs (UBI volume rootfs, /dev/ubi0_0): ..."), instead of only
    being counted. "No partition found" is no longer reported as "1
    partition could not be read".
  • read_ubi_volume() is gone, with nothing left calling it.
  • http_test: a file-backed span arrives whole between in-memory ones,
    and a short file is ERR_SOURCE.

mtd: leave gluebi's UBI-volume devices out of the partitions

On a gk7205v510 with NAND, each UBI volume is also an MTD device
(gluebi: mtd3 kernel, mtd4 rootfs, mtd5 rootfs_data). The ones not
mounted opened here and were listed as partitions after the UBI
partition that already holds them: the report showed kernel twice
and the backup carried it twice. The mounted ones refuse O_RDWR and
were skipped. So what appeared depended on what was mounted. They are
MTD_UBIVOLUME, not flash, and are now skipped for every consumer.

Tested on hardware

On a lab gk7205v510 with 128 MB NAND, which has three UBI volumes (rootfs_data 84 MB, rootfs 32 MB, kernel 4 MB) and ~50 MB free:

  • Before (master): Not sent: 1 partition(s) of the flash could not be read, the same as the report from the Novatek NT98566 in ipctool upload bug #232.
  • After: Backing up 5 partitions, 124032 KB; 127,010,625 bytes reached a test receiver, and the camera's free RAM didn't move. Each block in the received backup matches sha1sum on the camera: mtdblock0, mtdblock1, /dev/ubi0_0..2.
  • The same camera showed the gluebi duplicate (kernel listed and backed up twice) before the last commit, and not after.
  • A NOR hi3516ev300: 5 partitions, 16 MB, each block matching its mtdblock.

Not tested: ipctool backup <file>'s streamed path on hardware (no room for 124 MB on the camera). http_test covers the shared streaming code.

mtd: all twelve of Novatek's partitions, and not its "all"

The NT98566 in #234 has 12 MTD partitions. /proc/mtd was read only up to 10, so encrypt (4 MB) and all were dropped without a word: the report said 124M on a 128 MB chip, and a backup would have lacked encrypt. The limit is now 20 (MAX_MTDBLOCKS). all, an alias for the whole chip, is left out using the offsets in sysfs, or by size where a kernel has no offsets. mtd_alias_test covers #234's layout both ways and a NOR layout. On both lab boards the report and backup are unchanged.

`ipctool upload` with a backup on a Novatek NT98566 with 128 MB of NAND
refused with "Not sent: 1 partition(s) of the flash could not be read".
Its rootfs is a 62.5 MB UBI volume, and the backup malloc()ed every UBI
volume whole and held them all at once -- more than the camera has
free. The sha1s in the same report were fine, because those stream.

- A UBI volume is now a span with a path: http_post() and save_file()
  read it in 64 KB pieces while sending or writing. Nothing else
  changes in the body or the file. A file that ends short of its length
  stops the upload with ERR_SOURCE, since the Content-Length is already
  sent.
- A partition that cannot be read is named, with the reason
  ("rootfs (UBI volume rootfs, /dev/ubi0_0): ..."), instead of only
  being counted. "No partition found" is no longer reported as "1
  partition could not be read".
- read_ubi_volume() is gone, with nothing left calling it.
- http_test: a file-backed span arrives whole between in-memory ones,
  and a short file is ERR_SOURCE.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Stream UBI volumes during backup uploads and file saves

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Stream large UBI volumes in 64 KB chunks instead of retaining them in RAM.
• Preserve backup formatting while identifying unreadable partitions and aborting incomplete
 uploads.
• Test mixed memory/file uploads and short-file failures; NAND camera validation remains pending.
Diagram

graph TD
  UBI[("UBI volume")] --> Spans["Backup spans"] --> Multipart["Multipart body"] --> HTTP["HTTP upload"] --> Receiver["Report receiver"]
  Spans --> Save["File save"] --> Backup[("Backup file")]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use one shared streaming reader for both output paths
  • ➕ Centralizes chunked reads and short-source handling.
  • ➖ Adds an abstraction to two output paths with different I/O and error behavior.

Recommendation: Keep path-backed spans: they reuse the existing span-based backup format and avoid retaining large volumes or open descriptors across upload redirects. A shared reader is worth considering only if more consumers need identical source handling.

Files changed (8) +136 / -42

Enhancement (2) +10 / -1
http.hExtend spans with file paths and source errors +7/-1

Extend spans with file paths and source errors

• Adds an optional path to span_t and defines ERR_SOURCE for incomplete file-backed uploads.

src/http.h

report.cPreserve backup source paths in multipart spans +3/-0

Preserve backup source paths in multipart spans

• Multipart assembly carries path-backed backup spans through to the HTTP uploader while keeping generated fields and headers memory-backed.

src/report.c

Bug fix (3) +98 / -11
backup.cRepresent UBI volumes by path and stream local saves +48/-6

Represent UBI volumes by path and stream local saves

• Backup enumeration records each UBI volume's device path and length instead of loading its contents into RAM, and names partitions that cannot be opened. Local backup saves read path-backed spans in 64 KB pieces while retaining the existing length headers.

src/backup.c

http.cStream file-backed spans into HTTP requests +37/-3

Stream file-backed spans into HTTP requests

• The POST body writer reads path-backed spans in 64 KB pieces alongside in-memory spans. It returns ERR_SOURCE if a file cannot be opened or ends before its declared length.

src/http.c

report_cmd.cDistinguish missing partitions and failed source uploads +13/-2

Distinguish missing partitions and failed source uploads

• The upload command reports when no partition was found separately from partitions that could not be read. It gives ERR_SOURCE a dedicated incomplete-upload message.

src/report_cmd.c

Refactor (2) +0 / -30
mtd.cRemove whole-volume UBI reader +0/-28

Remove whole-volume UBI reader

• Deletes the UBI helper that allocated and read an entire volume; the separate streaming SHA-1 reader remains.

src/mtd.c

mtd.hRemove obsolete UBI reader declaration +0/-2

Remove obsolete UBI reader declaration

• Removes the public declaration of the deleted whole-volume reader.

src/mtd.h

Tests (1) +28 / -0
http_test.cTest file-backed and truncated upload sources +28/-0

Test file-backed and truncated upload sources

• Loopback HTTP tests check that a file-backed span arrives between memory-backed spans and that a short file returns ERR_SOURCE.

src/http_test.c

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. UBI uploads invoke undefined behavior ✓ Resolved
Description
http_post() evaluates spans[i].data + off before checking whether the span reads from a file.
UBI backup spans have data == NULL, so every nonempty UBI volume reaches this undefined pointer
arithmetic during upload, even though from is subsequently replaced with the file buffer.
Code

src/http.c[R268-269]

+            const char *from = spans[i].data + off;
+            if (src >= 0) {
Evidence
The backup callback creates path-backed spans with NULL data, the report preserves their paths, and
the upload loop calculates a pointer from that NULL data before its file-backed branch.

src/backup.c[80-89]
src/report.c[75-79]
src/http.c[264-289]

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

## Issue description
File-backed spans have a NULL data pointer, but `http_post()` performs pointer arithmetic on it before selecting the file buffer.
## Fix Focus Areas
- src/http.c[268-289]
## Recommended Fix
Initialize `from` inside the appropriate branch: use `piece` for file-backed spans and calculate `spans[i].data + off` only for memory-backed spans.

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



Remediation recommended

2. Disk write failures blame the UBI volume ✓ Resolved
Description
save_file() sends both a failed fread() and a failed fwrite() to the same branch, which prints
Error reading followed by the source volume path. When the backup destination cannot accept more
data, the command reports a source-read failure instead of identifying the write failure.
Code

src/backup.c[R158-159]

+                if (!n || fwrite(piece, 1, n, fp) != n)
+                    break;
Evidence
The newly added condition breaks on a short fwrite(), while the sole error message after that
break identifies blocks[i].path as the failed read source.

src/backup.c[153-167]

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

## Issue description
The streamed save path reports a destination write failure as an error reading the UBI volume.
## Fix Focus Areas
- src/backup.c[153-167]
## Recommended Fix
Handle failed source reads and destination writes separately. Report the relevant path and error for each failure before returning.

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


Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/http.c Outdated
Comment thread src/backup.c Outdated
On a gk7205v510 with NAND, each UBI volume is also an MTD device
(gluebi: mtd3 kernel, mtd4 rootfs, mtd5 rootfs_data). The ones not
mounted opened here and were listed as partitions after the UBI
partition that already holds them: the report showed `kernel` twice
and the backup carried it twice. The mounted ones refuse O_RDWR and
were skipped. So what appeared depended on what was mounted. They are
MTD_UBIVOLUME, not flash, and are now skipped for every consumer.
The NT98566 in #234 has twelve MTD partitions; /proc/mtd was read up to
ten, so "encrypt" (4 MB at 0x7c00000) and "all" were dropped without a
word. The report said 124M for a 128 MB chip, and a backup would have
lacked "encrypt" while calling itself whole. MAX_MTD is now
MAX_MTDBLOCKS (20; restore indexes partitions by number up to that).

"all" -- 0 to the chip's end, after the partitions that cover it -- is
another name for the flash, and backing it up would put the chip in
twice. mtd_mark_aliases() leaves out a partition that contains another
whole, by the offsets in /sys/class/mtd/mtdN/offset, or, without them,
one whose size is the sum of all the others'. mtd_alias_test holds it
to #234's layout both ways, and to a NOR layout where nothing is an
alias. On the lab gk7205v510 NAND and hi3516ev300 NOR the report and
backup are unchanged.
@openipc-ai openipc-ai changed the title backup: stream UBI volumes instead of holding them in RAM backup: stream UBI volumes, back up all of Novatek's partitions (#234) Oct 9, 2026
@openipc-ai
openipc-ai merged commit 7050918 into master Oct 9, 2026
5 checks passed
@openipc-ai
openipc-ai deleted the backup-streams-ubi-volumes branch October 9, 2026 14:40
@openipc-ai openipc-ai mentioned this pull request Oct 9, 2026
@sansarus sansarus mentioned this pull request Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ipctool backup bug

1 participant