New function psa_hash_clone #18
No reviewers
Labels
No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-ARMmbed/mbed-crypto!18
Loading…
Reference in a new issue
No description provided.
Delete branch "psa-hash_clone"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Clone a hash operation.
Test good cases as part as multipart tests. Add new test functions for
the state machine.
@ -921,6 +921,33 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation,*/These two return value documentations seem to contradict each other.
@ -921,6 +921,33 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation,*/Do you mean
\p target_operation is activein the second\retval?@ -924,0 +932,4 @@* two objects are independent, i.e. subsequent calls involving one of* the objects do not affect the other object.** \param[in] source_operation The active hash operation to clone.It should be documented that this should be initialized and active.
@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation,return( PSA_SUCCESS );Usually, the context acted upon is listed as the first argument; I'd therefore prefer the destination context to come first.
@ -1927,0 +1952,4 @@PSA_ASSERT( psa_hash_clone( &op_source, &op_init ) );PSA_ASSERT( psa_hash_finish( &op_init,hash, sizeof( hash ), &hash_len ) );PSA_ASSERT( psa_hash_clone( &op_source, &op_finished ) );Looking at this, I noticed that the documentation doesn't indicate the abstract state of a hash context after it has been terminated (through
psa_hash_finish,psa_hash_abort, orpsa_hash_verify). From my understanding, it's initialized but not setup/active. If so, you might consider documenting it, e.g. here.@ -921,6 +921,33 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation,*/psa_status_t psa_hash_abort(psa_hash_operation_t *operation);/** Clone a hash operation.Everything else wouldn't make sense, but we might still mention here that the target context is active and in the same internal state as the source after a successful call to this function.
The code looks fine. I left a few suggestions on how to improve the documentation.
@ -924,0 +932,4 @@* two objects are independent, i.e. subsequent calls involving one of* the objects do not affect the other object.** \param[in] source_operation The active hash operation to clone.Isn't the current wording sufficent? Active implies initialized.
@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation,return( PSA_SUCCESS );There's no such convention in this API. There's a convention that input comes before output, and a convention that the context comes before other parameters. With an input context and an output context, the input context comes first.
@ -1927,0 +1952,4 @@PSA_ASSERT( psa_hash_clone( &op_source, &op_init ) );PSA_ASSERT( psa_hash_finish( &op_init,hash, sizeof( hash ), &hash_len ) );PSA_ASSERT( psa_hash_clone( &op_source, &op_finished ) );This isn't critical to understanding the current behavior, and other types of operation should use consistent terminology, so I'll do that in a later writing pass. Tracked internally at
https://github.com/ARMmbed/psa-crypto/issues/132
@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation,return( PSA_SUCCESS );Ok.
@ -924,0 +932,4 @@* two objects are independent, i.e. subsequent calls involving one of* the objects do not affect the other object.** \param[in] source_operation The active hash operation to clone.That was an oversight on my side, because I was expecting the wording to be "The XXX. Must be YYY." instead of "The YYY XXX". I'd prefer the former, but that's not a blocker.
@ -1927,0 +1952,4 @@PSA_ASSERT( psa_hash_clone( &op_source, &op_init ) );PSA_ASSERT( psa_hash_finish( &op_init,hash, sizeof( hash ), &hash_len ) );PSA_ASSERT( psa_hash_clone( &op_source, &op_finished ) );Agreed.
@ -921,6 +921,33 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation,*/psa_status_t psa_hash_abort(psa_hash_operation_t *operation);/** Clone a hash operation.I added a paragraph of English text that defines the behavior of the function.
@ -921,6 +921,33 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation,*/Yes. I fixed the copypasta.
@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation,return( PSA_SUCCESS );Style: spaces before and after the parentheses.
Found a minor style issue, but other than that it looks good to me!
CI was flaky and failed
DTLS proxy: 3d, gnutls server, fragmentation, a known issue.