| Message ID | 1437413322-14808-1-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/20/2015 11:28 AM, Daniel P. Berrange wrote: > The current unit test only verify the encryption API, which grammatical mismatch; you want either: tests only verify test only verifies > resulted in us missing a recently introduced bug in the > decryption API from commit d3462e3. It was fortunately > later discovered & fixed by commit bd0959 thanks to the Is a 6-byte SHA going to stay unambiguous long enough? git defaults to 7-byte to reduce chances of collisions by 16. > QEMU I/O tests for qcow2 encryption, but we should really > detect this directly in the crypto unit tests. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > tests/test-crypto-cipher.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > @@ -266,6 +265,25 @@ static void test_cipher(const void *opaque) > > g_assert_cmpstr(outtexthex, ==, data->ciphertext); > > + g_free(outtexthex); > + > + if (iv) { > + g_assert(qcrypto_cipher_setiv(cipher, > + iv, niv, > + &err) == 0); > + g_assert(err == NULL); g_assert(qcrypto_cipher_setiv(cipher, iv, niv, &error_abort); saves you a call to g_assert(). > + } > + g_assert(qcrypto_cipher_decrypt(cipher, > + ciphertext, > + outtext, > + nplaintext, > + &err) == 0); > + g_assert(err == NULL); and again But that's style, not correctness, so: Reviewed-by: Eric Blake <eblake@redhat.com>
On 20 July 2015 at 21:57, Eric Blake <eblake@redhat.com> wrote: > On 07/20/2015 11:28 AM, Daniel P. Berrange wrote: >> The current unit test only verify the encryption API, which > > grammatical mismatch; you want either: > tests only verify > test only verifies > >> resulted in us missing a recently introduced bug in the >> decryption API from commit d3462e3. It was fortunately >> later discovered & fixed by commit bd0959 thanks to the > > Is a 6-byte SHA going to stay unambiguous long enough? git defaults to > 7-byte to reduce chances of collisions by 16. Fortunately causality allows us to disambiguate by knowing that Daniel can only be referring to a commit that's already happened rather than one in the future :-) -- PMM
On 07/20/2015 03:37 PM, Peter Maydell wrote: > On 20 July 2015 at 21:57, Eric Blake <eblake@redhat.com> wrote: >> On 07/20/2015 11:28 AM, Daniel P. Berrange wrote: >>> The current unit test only verify the encryption API, which >> >> grammatical mismatch; you want either: >> tests only verify >> test only verifies >> >>> resulted in us missing a recently introduced bug in the >>> decryption API from commit d3462e3. It was fortunately >>> later discovered & fixed by commit bd0959 thanks to the >> >> Is a 6-byte SHA going to stay unambiguous long enough? git defaults to >> 7-byte to reduce chances of collisions by 16. > > Fortunately causality allows us to disambiguate by knowing > that Daniel can only be referring to a commit that's > already happened rather than one in the future :-) True, but that doesn't mean git makes it easy to disambiguate. When, in the future, resolving the short name becomes ambiguous, how do you coax git into showing the full SHA of both (or all) candidates, as well as the date they were created, so you can then compare dates to the commit message you are reading and deduce which one was meant? $ git rev-parse ffff error: short SHA1 ffff is ambiguous. ffff error: short SHA1 ffff is ambiguous. fatal: ambiguous argument 'ffff': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' /me goes and reads the man page Hmm - this is something I've never used before: $ git rev-parse --disambiguate=ffff ffffb8c0bd544ab479a7a32a208d9de13aff62d9 ffffbf79aa11e013e8fd14ffb134faee6489b42c ffffff01466a0180de7632842cf583c8a9cbf959 ffffbb369f3ed9bca5ff2867143f76d0c6e069c0 But that still doesn't tell you that two of those are blobs, and two of them commits, nor what dates the commits have, in a way that can easily determine if one of the commits was an ancestor of the point where a (hypothetical) commit message referred to a (once-non-ambiguous) short reference. And (straying off-topic for this list) why does 'git rev-parse --disambiguate ffff' not behave the same as 'git rev-parse --disambiguate=ffff'?
diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c index f9b1a03..4485886 100644 --- a/tests/test-crypto-cipher.c +++ b/tests/test-crypto-cipher.c @@ -231,7 +231,6 @@ static void test_cipher(const void *opaque) size_t nkey, niv, nciphertext, nplaintext; char *outtexthex; - g_test_message("foo"); nkey = unhex_string(data->key, &key); niv = unhex_string(data->iv, &iv); nciphertext = unhex_string(data->ciphertext, &ciphertext); @@ -266,6 +265,25 @@ static void test_cipher(const void *opaque) g_assert_cmpstr(outtexthex, ==, data->ciphertext); + g_free(outtexthex); + + if (iv) { + g_assert(qcrypto_cipher_setiv(cipher, + iv, niv, + &err) == 0); + g_assert(err == NULL); + } + g_assert(qcrypto_cipher_decrypt(cipher, + ciphertext, + outtext, + nplaintext, + &err) == 0); + g_assert(err == NULL); + + outtexthex = hex_string(outtext, nplaintext); + + g_assert_cmpstr(outtexthex, ==, data->plaintext); + g_free(outtext); g_free(outtexthex); g_free(key);
The current unit test only verify the encryption API, which resulted in us missing a recently introduced bug in the decryption API from commit d3462e3. It was fortunately later discovered & fixed by commit bd0959 thanks to the QEMU I/O tests for qcow2 encryption, but we should really detect this directly in the crypto unit tests. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/test-crypto-cipher.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)