[fix] Part init failure with multiple init calls#240
[fix] Part init failure with multiple init calls#240rajesh-gali wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes behavior around partition initialization when init is called multiple times, ensuring subsequent operations behave consistently after an init attempt.
Changes:
- Updated
HsmPartition::initto avoid propagatingddi::init_parterrors and instead log them. - Adjusted partition init tests to expect
initto succeed, and to validate failure viaopen_sessioninstead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| api/tests/src/partition_tests.rs | Updates init-related negative tests to treat init as non-failing and validate failure through open_session. |
| api/lib/src/partition.rs | Changes HsmPartition::init to swallow init errors (log + return Ok(())). |
| let creds = HsmCredentials::new(&APP_ID, &APP_PIN); | ||
| part.init(creds, None, None, obk_config, pota) | ||
| .expect("init should return Ok even on bad params"); | ||
|
|
||
| let rev = part.api_rev_range().max(); | ||
| let result = part.open_session(rev, &creds, None); | ||
| assert!( | ||
| result.is_err(), | ||
| "open_session should fail after init with null OBK" | ||
| ); |
There was a problem hiding this comment.
These negative tests became significantly less specific: they now only assert open_session returns some error and no longer verify the expected error variant (previously InvalidArgument). This can hide regressions (e.g., failing for the wrong reason). Consider asserting the exact error returned by open_session (or by init, depending on the intended contract), and apply the same strengthening across the similar updated tests in this file.
| pota, | ||
| let creds = HsmCredentials::new(&APP_ID, &APP_PIN); | ||
| part.init(creds, None, None, obk_config, pota) | ||
| .expect("init should return Ok even on bad params"); |
There was a problem hiding this comment.
The panic message is misleading/unhelpful for diagnosing failures: it states that init should succeed 'even on bad params', which reads like an unconditional success requirement rather than the specific scenario being tested. Consider updating it to describe the intended behavior more precisely (e.g., init should be idempotent / should not error even if later session creation is rejected).
| .expect("init should return Ok even on bad params"); | |
| .expect("init should succeed even when caller supplies a null OBK; session creation is expected to fail later"); |
part init changes to allow partition init success for already established credentials
rajesh-gali
force-pushed
the
user/rajeshgali/hsm-part-init-fix
branch
from
01643c1 to
ccddd45
Compare
Addressed an issue where subsequent part init fails after a successful init.