Skip to content

fix: address Preloop review findings for RAM staging - #1

Closed
ajlennon wants to merge 1 commit into
feature/zephyr-ram-stage-on-mainfrom
fix/preloop-review-findings
Closed

fix: address Preloop review findings for RAM staging#1
ajlennon wants to merge 1 commit into
feature/zephyr-ram-stage-on-mainfrom
fix/preloop-review-findings

Conversation

@ajlennon

@ajlennon ajlennon commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Gate Zephyr zephyr-image RAM staging behind CONFIG_MENDER_ZEPHYR_IMAGE_RAM_STAGE (+ optional max-bytes cap); use mender_malloc/mender_free via shared helpers with unit tests; keep NULL-handle checks and direct-to-flash fallback.
  • Fix ESP-IDF MENDER_STORAGE_PARTITION_LABEL cmake/macro mismatch; grow ESP-IDF log formatting past a fixed 256-byte stack buffer.
  • Parameterize pin scripts (MENDER_MCU_PIN_BRANCH, DD_ROOT, MENDER_MCU_CONSUMERS); document RAM staging in README/CHANGELOG.

Test plan

  • tests/unit ImageRamStage* (5 tests) pass locally
  • Consumer native_sim / FRDM smoke after pin bump (not required to merge this stacked PR onto the feature branch)

AI disclosure

This PR was AI-assisted (Cursor).

Made with Cursor

Note

[Low Risk]
No security issues; the changes gate RAM staging behind a config, refactor staging into shared tested helpers, and fix an ESP-IDF macro/build mismatch. Only a minor, non-blocking quality suggestion applies.

Overview
This PR addresses prior review findings for RAM staging on Zephyr: adds config gating and a size cap, extracts staging into mender_image_ram_stage_* helpers backed by mender_malloc/mender_free with unit tests and direct-to-flash fallback, and fixes the ESP-IDF MENDER_STORAGE_PARTITION_LABEL build/macro mismatch plus oversized log formatting.

Written by Preloop PR Reviewer for commit 0da7502. Updates automatically on new commits.

Gate Zephyr RAM staging behind Kconfig, use mender_malloc, add unit-tested
helpers, and fix the ESP-IDF partition-label cmake mismatch plus log truncation.

Co-authored-by: Cursor <cursoragent@cursor.com>

@preloop preloop Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Preloop approves this PR.

All previously raised concerns have been addressed with a clean, well-tested implementation. A couple of minor (LOW) suggestions below, but the code is ready to merge.

See the summary comment for the full review details.

return MENDER_FAIL;
}

mender_image_ram_stage_reset(stage);

@preloop preloop Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] [Quality]

Affected files:

  • src/core/image-ram-stage.c:31-46

mender_image_ram_stage_begin unconditionally calls mender_image_ram_stage_reset(stage), which mender_frees stage->buf. If a caller ever passes a non-zero-initialized struct (e.g. a stack mender_image_ram_stage_t stage; without = {0}), reset could free a garbage pointer. The only current caller is a zero-initialized static, and the unit tests use = {}, so this is latent rather than active.

Recommendation:
Document in the header that the struct must be zero-initialized before begin, or only free when buf != NULL after an internal init guard.


⏳ Still open as of commit 0da7502 — this issue has not been addressed yet.

@preloop

preloop Bot commented Aug 4, 2026

Copy link
Copy Markdown

🔍 Preloop Code Review

Last Updated: 2026-08-04
Reviewing Commit: 0da7502
Review Status: ✅ Approved


📝 Summary

This PR addresses prior review findings: it gates Zephyr zephyr-image RAM staging behind CONFIG_MENDER_ZEPHYR_IMAGE_RAM_STAGE (+ optional CONFIG_MENDER_ZEPHYR_IMAGE_RAM_STAGE_MAX_BYTES cap), refactors staging into shared image-ram-stage.c/.h helpers using mender_malloc/mender_free with unit tests, fixes the ESP-IDF MENDER_STORAGE_PARTITION_LABEL cmake/macro mismatch, grows ESP-IDF log formatting past the fixed 256-byte stack buffer, and parameterizes the pin scripts. The implementation is clean, well-tested, and documented.

✅ What Looks Good

  • RAM staging properly gated behind Kconfig with a max-bytes cap and direct-to-flash fallback on alloc failure.
  • Staging logic extracted into platform-independent helpers with NULL-handle checks and unit tests.
  • Memory ownership is clear: buffers freed via mender_image_ram_stage_reset on close/abort paths.
  • ESP-IDF log now heap-backs oversize messages with a cap and clean mender_free.
  • ESP-IDF MENDER_STORAGE_PARTITION_LABEL quoting now matches storage.c s #ifdef.
  • README and CHANGELOG updated for the new Kconfig options.

⚠️ Issues Found

🟢 Low Priority

  • [Quality]: mender_image_ram_stage_begin frees stage->buf via reset before validating initialization, which could free a garbage pointer if a caller passes a non-zero-initialized struct. Latent today (all callers zero-init); recommend documenting zero-init requirement. - src/core/image-ram-stage.c:31

✅ Resolved Issues

None — first review pass on this PR.


📄 Documentation Impact

Documentation was already included in this PR (README + CHANGELOG entries for RAM staging). No additional doc gaps identified.


Progress: 1 low-priority suggestion; no blocking issues. PR is ready to merge.

This summary updates automatically on each review. Inline comments provide detailed feedback on specific lines.

@preloop preloop Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Preloop approves this PR.

A few minor suggestions below, but the code is ready to merge.

See the summary comment for details.

@ajlennon

ajlennon commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Superseded by recheck PR with the same commit for a clean Preloop trigger.

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.

1 participant