Fix useless calls in PSA crypto metadata tests #378

2020-02-28T02:51:13-06:00

ronald-cron-arm commented

2020-02-28 02:51:13 -06:00

(Migrated from github.com)

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 ronald.cron@arm.com

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 <ronald.cron@arm.com>

gilles-peskine-arm (Migrated from github.com) requested changes

2020-03-02 06:33:46 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Given the small size of the patch and the fact that the commit needs to be split anyway, please force-push a new history.

tests/suites/test_suite_psa_crypto_metadata.function

					
				@ -56,8 +56,13 @@

				#define TEST_CLASSIFICATION_MACRO( flag, alg, flags )           \

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 https://github.com/ARMmbed/mbed-crypto/pull/330#discussion_r360305364 . 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 )

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 https://github.com/ARMmbed/mbed-crypto/pull/330#discussion_r360305364 . 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 ) ```

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.

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.

ronald-cron-arm (Migrated from github.com) reviewed

2020-03-02 09:13:15 -06:00

tests/suites/test_suite_psa_crypto_metadata.function

					
				@ -56,8 +56,13 @@

				#define TEST_CLASSIFICATION_MACRO( flag, alg, flags )           \

I've changed the function documentation taking into account your proposal. Please have a look to the new version.

ronald-cron-arm (Migrated from github.com) reviewed

2020-03-02 09:16:44 -06:00

tests/suites/test_suite_psa_crypto_metadata.function

					
				@ -56,8 +56,13 @@

				#define TEST_CLASSIFICATION_MACRO( flag, alg, flags )           \

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.