Change persistent storage SyncGetKeyValue API to match constraints of KvsStorageManager #20164
Conversation
This file contains 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
…e size Some implmentation of PersistentStorageDelegate use KeyValueStoreManager which is not capable of giving the size of for the time being, which makes this obligation impossible to fulfill right now. After performing a code audit no one is currently using the functionality to get the size, so this change is safe, but we need platforms to adhear to the this updated description. We also are now enforcing the PersistentStorageDelegate::SyncGetKeyValue API such that we fill up the buffer up to the size provided even if the buffer is not large enough. This is done so that in the case a version downgrade needs to happen that (due to OTA revert, security reason, or any other reason) we can make sure as long as TLV data was stored you could still ensure that you are able to work in critical situations.
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs and
jtung-apple
|
PR #20164: Size comparison from 31ddf98 to c182809 Increases (10 builds for cyw30739, k32w, linux, mbed, telink)
Full report (10 builds for cyw30739, k32w, linux, mbed, telink)
|
Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
|
PR #20164: Size comparison from 31ddf98 to 9475174 Increases above 0.2%:
Increases (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
tcarmelveilleux
approved these changes
Jun 30, 2022
bzbarsky-apple
approved these changes
Jun 30, 2022
Co-authored-by: Boris Zbarsky <[email protected]>
This was referenced Aug 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
PersistentStorageDelegate::SyncGetKeyValueand KeyValueStoreManager #20163Change overview
sizeinput/output argument is set to the total size of the value whenCHIP_ERROR_BUFFER_TOO_SMALLis returned.CHIP_ERROR_BUFFER_TOO_SMALLis returned bySyncGetKeyValue, we expect that thebufferwill be populated up to thesizeprovided.SyncGetKeyValue(which matches 1:1 to KeyValueStoreManager), we cannot actually “read however much we support” and try to deal with it. It would always error-out, making it harder to actually implement and test “upgrade/downgrade-safe” code.Testing
scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone chip_config_network_layer_ble=false chip_support_enable_storage_api_audit=true./out/debug/standalone/chip-all-clusters-app