CP: Add Writability to Storage Abstraction (#5133) #5165
Conversation
* Add Writability to Storage Abstraction * Add tests from #5132 --------- Co-authored-by: Guillaume Hetier <guhetier@microsoft.com>
|
Just to understand, why do we need to cherry-pick this to 2.5? (Could be nice to add in the PR description for posterity) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/2.5 #5165 +/- ##
===============================================
- Coverage 87.31% 85.69% -1.62%
===============================================
Files 59 59
Lines 18048 18048
===============================================
- Hits 15758 15467 -291
- Misses 2290 2581 +291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Your "retry" PR made some extra changes to the "clear" function.
|
|
The change looks good to me, but can you check the test failure is unrelated? |
yes, that PR will also be cherry-picked once it is completed. |
| // | ||
| _IRQL_requires_max_(PASSIVE_LEVEL) | ||
| QUIC_STATUS | ||
| CxPlatStorageWriteValue( |
There was a problem hiding this comment.
I dont recall if we discussed this already.
These are quite risky functions from production perspective. We should surround these declarations with a #define and allow the functions to be used in the test code for now. If we need it in the msquic lib, we can discuss each usage specifically.
There was a problem hiding this comment.
SettingsTest.cpp uses QUIC_UNIT_TESTS macro for this.
| StorageName, | ||
| nullptr, | ||
| nullptr, | ||
| CXPLAT_STORAGE_OPEN_FLAG_DELETE | CXPLAT_STORAGE_OPEN_FLAG_WRITE | CXPLAT_STORAGE_OPEN_FLAG_CREATE, |
There was a problem hiding this comment.
Nit: Perhaps this line is too long?
A scope-local param "flags" could be set to these flags and used instead for better readability.
| @@ -73,6 +79,34 @@ ZwNotifyChangeKey( | |||
| _In_ BOOLEAN Asynchronous | |||
| ); | |||
|
|
|||
| // | |||
| // Copied from wdm.h | |||
There was a problem hiding this comment.
We have a couple of references to wdm.h here. Just curious if it is a public file.
| @@ -5980,549 +5980,343 @@ QuicTestCredentialLoad(const QUIC_CREDENTIAL_CONFIG* Config) | |||
| TEST_QUIC_SUCCEEDED(Configuration.LoadCredential(Config)); | |||
| } | |||
|
|
|||
|
|
|||
| class QuicStorageSettingScopeGuard { | |||
There was a problem hiding this comment.
Perhaps a comment to mention this is duplicated in the settings test as well...
| StorageName, | ||
| nullptr, | ||
| nullptr, | ||
| CXPLAT_STORAGE_OPEN_FLAG_DELETE | CXPLAT_STORAGE_OPEN_FLAG_WRITE | CXPLAT_STORAGE_OPEN_FLAG_CREATE, |
There was a problem hiding this comment.
Please see my comment in the settings unit test code.
| @@ -294,50 +334,84 @@ CxPlatStorageOpen( | |||
|
|
|||
| CxPlatZeroMemory(Storage, sizeof(CXPLAT_STORAGE)); | |||
| CxPlatLockInitialize(&Storage->Lock); | |||
| Storage->Callback = Callback; | |||
| Storage->CallbackContext = CallbackContext; | |||
| if (Callback != NULL) { | |||
There was a problem hiding this comment.
Should we also validate or assert that CallbackContext is not NULL?
| @@ -121,99 +126,136 @@ CxPlatStorageOpen( | |||
| } | |||
|
|
|||
| CxPlatZeroMemory(Storage, sizeof(CXPLAT_STORAGE)); | |||
| Storage->Callback = Callback; | |||
| Storage->CallbackContext = CallbackContext; | |||
| if (Callback != NULL) { | |||
There was a problem hiding this comment.
Should we also check/assert if CallbackContext is not NULL as well?
Add Writability to Storage Abstraction
Add tests from Additional settings tests and misc changes #5132