Skip to content

Fix max_keys reset limitation #9054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Conversation

theamirocohen
Copy link
Contributor

@theamirocohen theamirocohen commented Dec 11, 2018

Fix issue NvStore does not correctly persist values through reset
Resolves #8785
The former NvStore implementation did not persist the max_keys value through a soft-reset, a set value would have disappear after reset, meaning NvStore would start over with its default max_keys value which may cause data to be lost.
We added the max_keys value to master record (written to non volatile area) so we can read and update its value after reset.
Furthermore we added the restriction of setting max_keys to be bigger than the default predefined keys value (16), and in case we have set max_keys to a new value and want to set it again to a smaller value the operation will fail if there are keys that might be lost (if keys were store at that gap).

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@ciarmcom ciarmcom requested review from a team December 11, 2018 14:00
@ciarmcom
Copy link
Member

@theamirocohen, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@yossi2le yossi2le left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 12, 2018

@theamirocohen Can you add more details to the commit msg ? The fix does not look that trivial.

cc @NeilMacMullen for review

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 12, 2018

I set this to 5.12 but I noticed this is updating private method, so no functional change there (fix can go to the next patch) ?

@theamirocohen
Copy link
Contributor Author

I set this to 5.12 but I noticed this is updating private method, so no functional change there (fix can go to the next patch) ?

Yes, fix can go to next patch

Persist the max_keys value through a soft-reset, also prohibit max_keys set under predefined default value (16)
@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 12, 2018

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

Interesting. Error indicates that the NRF52_DK failed to flash.

CI job restarted: jenkins-ci/greentea-test

@cmonr cmonr merged commit 839e005 into ARMmbed:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants