Fix useless calls in PSA crypto metadata tests #378
Conversation
ronald-cron-arm
added
bug
| /* Check the parity of value. | ||
| * Return 0 if value has even parity and a nonzero value otherwise. */ | ||
| int test_parity( uint32_t value ) | ||
| /* Check if the parity of value is even. |
There was a problem hiding this comment.
Please comment the why in preference to the what. The job of this function is to test whether the parity is whichever parity is expected for numerical values. The fact that it's even parity is a detail.
See the discussion in #330 (comment) . This discussion also highlights that “even parity” is not such good documentation: Jaeden calls it “odd parity”!
My bad for not documenting why we're checking the parity of some values. Here's some proposed documentation.
/* Check the parity of value.
* Numerical encodings of key types and related values such as EC curves and
* DH groups should all have the same parity. This guarantees that a
* single-bit error will be detected.
* The expected parity is even because this makes all-bits-zero valid.
*/
int test_parity( uint32_t value )
There was a problem hiding this comment.
I've changed the function documentation taking into account your proposal. Please have a look to the new version.
| /* Check the parity of value. | ||
| * Return 0 if value has even parity and a nonzero value otherwise. */ | ||
| int test_parity( uint32_t value ) | ||
| /* Check if the parity of value is even. |
There was a problem hiding this comment.
Please don't combine unrelated things into the same commit. Here you have a commit that does four things: adding documentation, renaming a function, changing the return value, and adding the missing TEST_ASSERT calls. Adding documentation could go with changing the return value but the other three should be separate.
There was a problem hiding this comment.
I don't rename the function and don't change the return value anymore. Two commits now: the first one fix/improve the documentation of the test_parity() function. The second one fixes the useless calls.
gilles-peskine-arm
added
needs: work
ronald-cron-arm
force-pushed
the
psa_crypto_metadata-test_parity
branch
from
2563f92 to
685764f
Compare
Fix and improve the documentation of the test_parity() function in PSA crypto metadata tests: . Align the short description of the function with the function name: "Test ..." instead of "Check ..." . Fix the description of the returned value: the returned value is nonzero, not zero, if the parity is even. . Add documentation explaining the needs for parity testing and that the parity is expected to be even. Signed-off-by: Ronald Cron <[email protected]>
In ecc_key_family test, the parity of the elliptic curve identifier was computed but not tested to be even. In dh_key_family test, the parity of the PSA Diffie-Hellman group identifier was computed but not tested to be even. Signed-off-by: Ronald Cron <[email protected]>
ronald-cron-arm
force-pushed
the
psa_crypto_metadata-test_parity
branch
from
685764f to
3876f4a
Compare
ronald-cron-arm
added
needs: review
|
Everything passed in the CI except for mbed-os and iar tests which are known issues. |
mpg
added
ready for merge
In ecc_key_family test, the parity of the elliptic curve identifier
was computed but not tested to be even.
In dh_key_family test, the parity of the PSA Diffie-Hellman group
identifier was computed but not tested to be even.
Signed-off-by: Ronald Cron [email protected]