fix: preserve unrecognized, chunked, and cross-page NVS entries on read-modify-write - #50
Merged
Conversation
…ad-modify-write
ParseNVS silently dropped or truncated anything it didn't fully model, so
GenerateNVS's re-encode of a parsed partition would wipe real data on a
read-modify-write:
- unknown/unmodeled entry types hit `default: continue` and were dropped
entirely (e.g. any future or vendor NVS type)
- ESP-IDF's blob-index + chunked blob-data entries (used for esp_wifi
credentials) share one key across several chunk-indexed slots; keying
dedup on namespace+key alone collapsed all chunks into one, discarding
every chunk but the last scanned
- string/blob spans that cross a page boundary were truncated at "assume
fits in current page", even though GenerateNVS's own writePage already
produces such spans for long values
- a key whose namespace declaration lived on a later-scanned page than the
key itself was dropped ("namespace not yet defined")
- GenerateNVS ranged over a Go map to pick namespace write order, making
output nondeterministic across otherwise-identical calls
Fixes:
- parse.go: two-phase parse — a structural walk decodes every entry record
(namespace declarations resolved immediately, everything else queued),
then a resolution phase looks up namespace names against the now-complete
map, so scan order can no longer drop a key
- parse.go: readSpanData follows an entry's continuation slots across page
boundaries instead of stopping at the current page's last slot
- parse.go: entries of a type the switch doesn't decode are captured
generically (Entry.Raw/TypeByte/Span/ChunkIndex/Data) instead of dropped
- parse.go: dedup key is (namespace, key, chunkIndex) instead of
(namespace, key), so chunked entries sharing a key survive independently
- generate.go: buildRawEntry re-emits Raw entries byte-for-byte from their
captured type/span/chunkIndex/data, without needing to understand the
value's semantics
- generate.go: namespace write order follows first-seen order in the input
slice instead of Go map iteration, making output deterministic
Entry gains Raw/TypeByte/Span/ChunkIndex/Data fields, additive only;
ParseNVS/GenerateNVS signatures are unchanged.
Adds pkg/nvs/lossless_test.go with hand-built raw NVS page fixtures
covering: namespace declared on a later page than its key, unknown-type
passthrough, blob-index/chunked-blob round trip, a blob spanning a page
boundary, GenerateNVS determinism, and a full parse->modify-one-key->
generate->reparse round trip asserting every other entry is
byte-preserved.
Member
|
Thanks for the fixes here @jgangemi now merging. |
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
2 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.
Problem
nvs.ParseNVSsilently dropped any entry it didn't fully decode — unrecognized entry types, blob-index / blob-data chunks, and multi-slot items — returning only the recognized subset. Round-tripping a partition throughParseNVS→GenerateNVS(a read-modify-write) therefore re-encoded only the survivors, destroying the dropped entries (e.g. Wi-Fi credentials, chunked blobs) while reporting success.Fix
Page::writeItem(which bumps an oversized item wholly to the next page) — removes an earlier incorrect cross-page carry model.Validation
nvs_partition_gen(string + int namespaces plus a 6144-byte chunked blob = BLOB_IDX + 2 BLOB_DATA chunks) byte-identically — added as an in-tree fixture +TestParseNVSRealESPIDFBlobPartitionRoundTrips.