Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deprecate broken TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP in kv_config #14657

Merged
merged 2 commits into from
May 13, 2021
Merged

Deprecate broken TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP in kv_config #14657

merged 2 commits into from
May 13, 2021

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented May 11, 2021 •
edited
Loading

Summary of changes

Replaces #14490: changed from removal to deprecation due to the Mbed OS release model.

NO_RBP (no rollback protection) is intended to not require an internal TDB, however, DeviceKey, which we use to derive SecureStore's encryption key, still does. Currently, no internal TDB is created with these two configurations, meaning there's no way to store the DeviceKey and SecureStore doesn't work. The configurations TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP are broken due to this design flaw.

This PR deprecates the non-working configurations, with their documentations removed but code and configurations kept with deprecation warnings. They will be completely removed from the next major release.

Impact of changes

Existing applications that use TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP will still be able to compile for now, until we fully remove the two configurations in the future. But they have never been functional, so no application should have ever been able to use them.

Migration actions required

Applications that have storage.storage_type set to TDB_EXTERNAL_NO_RBP or FILESYSTEM_NO_RBP should switch to use TDB_INTERNAL, TDB_EXTERNAL, FILESYSTEM or default depending on their use cases.

Documentation

ARMmbed/mbed-os-5-docs#1440 raised to remove references to TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP from the online documentation.

Pull request type 

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-core

@harrisonmutai-arm @LDong-Arm
Deprecate TDB_EXTERNAL_NO_RBP and FILESYSTEM_NO_RBP in kv_config
468ee97 
NO_RBP (no rollback protection) is intended to not require an internal
TDB, however, DeviceKey, which we use to derive SecureStore's
encryption key, still does. Currently, no internal TDB is created with
these two configurations, meaning there's no way to store the DeviceKey
and SecureStore doesn't work.
@harrisonmutai-arm @LDong-Arm
Refactor KVStore config override explanation
6b2c133 
The documentation previously referred to a weakly defined function
`storage_configuration`, however, this function was replaced at some
stage by `kv_init_storage_config`. Refactor the explanation on how to
override the default configurations to reflect this. Also, remove
the snippet which was used to show the implentation of
`storage_configuration`.
@LDong-Arm LDong-Arm mentioned this pull request May 11, 2021
Closed
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 11, 2021
@ciarmcom ciarmcom requested a review from a team May 11, 2021 16:30
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented May 12, 2021 •
edited
Loading

Functionality change

I would mark this as functionality change, although it could be patch. A reason: adding this to the release notes automatically.
To make it more visible this is being deprecated.

The changes look good to me.

@LDong-Arm
Copy link
Contributor Author

I would mark this as functionality change, although it could be patch. A reason: adding this to the release notes automatically.
To make it more visible this is being deprecated.

Now changed.

Patater
Patater approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot added needs: CI and removed needs: review labels May 12, 2021
@adbridge
Copy link
Contributor

adbridge commented May 13, 2021 •
edited
Loading

@LDong-Arm as this is a functionality change please also fill in :
Impact of changes
Migration actions required
Sections.

@LDong-Arm
Copy link
Contributor Author

@LDong-Arm as this is a functionality change please also fill in :
Impact of changes
Migration actions required
Sections.

Updated

@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

@ciarmcom ciarmcom added the stale Stale Pull Request label May 13, 2021
@adbridge
Copy link
Contributor

Ci started

@mbed-ci
Copy link

mbed-ci commented May 13, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@adbridge adbridge merged commit 7279ae2 into ARMmbed:master May 13, 2021
@adbridge adbridge removed ready for merge stale Stale Pull Request release-type: patch Indentifies a PR as containing just a patch labels May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbridge adbridge adbridge approved these changes

@Patater Patater Patater approved these changes

Assignees
No one assigned
Labels
component: core component: docs release-version: 6.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LDong-Arm @ciarmcom @0xc0170 @adbridge @mbed-ci @Patater @mbedmain @harrisonmutai-arm