[PATCH] unit: check cipher support and cleanup in test-storage

[PATCH] unit: check cipher support and cleanup in test-storage

[James Prestwood]

The __storage_decrypt API requires AES_CTR so support should be
checked before running that test. In addition storage_exit was never
being called which leaves unbalanced mlock/munlock calls.
---
 unit/test-storage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/unit/test-storage.c b/unit/test-storage.c
index c40518e6..765d9967 100644
--- a/unit/test-storage.c
+++ b/unit/test-storage.c
@@ -44,12 +44,19 @@ static void test_short_encrypted_bytes(const void *data)
 
 int main(int argc, char *argv[])
 {
+	int ret;
+
 	l_test_init(&argc, &argv);
 
 	storage_init((const uint8_t *)"abc123", 6);
 
-	l_test_add("/storage/profile encryption",
+	if (l_cipher_is_supported(L_CIPHER_AES_CTR))
+		l_test_add("/storage/profile encryption",
 			test_short_encrypted_bytes, NULL);
 
-	return l_test_run();
+	ret = l_test_run();
+
+	storage_exit();
+
+	return ret;
 }
-- 
2.34.1

Re: [PATCH] unit: check cipher support and cleanup in test-storage

[Marcel Holtmann]

Hi James,

> The __storage_decrypt API requires AES_CTR so support should be
> checked before running that test. In addition storage_exit was never
> being called which leaves unbalanced mlock/munlock calls.
> ---
> unit/test-storage.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/unit/test-storage.c b/unit/test-storage.c
> index c40518e6..765d9967 100644
> --- a/unit/test-storage.c
> +++ b/unit/test-storage.c
> @@ -44,12 +44,19 @@ static void test_short_encrypted_bytes(const void *data)
> 
> int main(int argc, char *argv[])
> {
> + int ret;
> +
> l_test_init(&argc, &argv);
> 
> storage_init((const uint8_t *)"abc123", 6);
> 
> - l_test_add("/storage/profile encryption",
> + if (l_cipher_is_supported(L_CIPHER_AES_CTR))
> + l_test_add("/storage/profile encryption",
> test_short_encrypted_bytes, NULL);

just use L_TEST_FLAG_ALLOW_FAILURE instead.

> - return l_test_run();
> + ret = l_test_run();
> +
> + storage_exit();
> +
> + return ret;

I really prefer we keep “return l_test_run()” as the basics on how test case are run.

Just put storage_init,storage_exit into the test case itself. I would just do system_key_set = false in the exit function.

Regards

Marcel

Re: [PATCH] unit: check cipher support and cleanup in test-storage

[James Prestwood]

Hi,

On 5/5/25 5:39 AM, Marcel Holtmann wrote:
> Hi James,
>
>> The __storage_decrypt API requires AES_CTR so support should be
>> checked before running that test. In addition storage_exit was never
>> being called which leaves unbalanced mlock/munlock calls.
>> ---
>> unit/test-storage.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/unit/test-storage.c b/unit/test-storage.c
>> index c40518e6..765d9967 100644
>> --- a/unit/test-storage.c
>> +++ b/unit/test-storage.c
>> @@ -44,12 +44,19 @@ static void test_short_encrypted_bytes(const void *data)
>>
>> int main(int argc, char *argv[])
>> {
>> + int ret;
>> +
>> l_test_init(&argc, &argv);
>>
>> storage_init((const uint8_t *)"abc123", 6);
>>
>> - l_test_add("/storage/profile encryption",
>> + if (l_cipher_is_supported(L_CIPHER_AES_CTR))
>> + l_test_add("/storage/profile encryption",
>> test_short_encrypted_bytes, NULL);
> just use L_TEST_FLAG_ALLOW_FAILURE instead.
Ok, new way of doing things I guess.
>
>> - return l_test_run();
>> + ret = l_test_run();
>> +
>> + storage_exit();
>> +
>> + return ret;
> I really prefer we keep “return l_test_run()” as the basics on how test case are run.
>
> Just put storage_init,storage_exit into the test case itself. I would just do system_key_set = false in the exit function.

I'm fine with this, but its actually not possible to perform the cleanup 
after a failed test. If we want to do this we need actual cleanup 
support in l_test. Looks like we'd need to add a destroy function to 
l_test_add_data_func().

Does that sound ok?

Thanks,

James

>
> Regards
>
> Marcel
>

Re: [PATCH] unit: check cipher support and cleanup in test-storage

[Marcel Holtmann]

Hi James,

>>> The __storage_decrypt API requires AES_CTR so support should be
>>> checked before running that test. In addition storage_exit was never
>>> being called which leaves unbalanced mlock/munlock calls.
>>> ---
>>> unit/test-storage.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/unit/test-storage.c b/unit/test-storage.c
>>> index c40518e6..765d9967 100644
>>> --- a/unit/test-storage.c
>>> +++ b/unit/test-storage.c
>>> @@ -44,12 +44,19 @@ static void test_short_encrypted_bytes(const void *data)
>>> 
>>> int main(int argc, char *argv[])
>>> {
>>> + int ret;
>>> +
>>> l_test_init(&argc, &argv);
>>> 
>>> storage_init((const uint8_t *)"abc123", 6);
>>> 
>>> - l_test_add("/storage/profile encryption",
>>> + if (l_cipher_is_supported(L_CIPHER_AES_CTR))
>>> + l_test_add("/storage/profile encryption",
>>> test_short_encrypted_bytes, NULL);
>> just use L_TEST_FLAG_ALLOW_FAILURE instead.
> Ok, new way of doing things I guess.
>> 
>>> - return l_test_run();
>>> + ret = l_test_run();
>>> +
>>> + storage_exit();
>>> +
>>> + return ret;
>> I really prefer we keep “return l_test_run()” as the basics on how test case are run.
>> 
>> Just put storage_init,storage_exit into the test case itself. I would just do system_key_set = false in the exit function.
> 
> I'm fine with this, but its actually not possible to perform the cleanup after a failed test. If we want to do this we need actual cleanup support in l_test. Looks like we'd need to add a destroy function to l_test_add_data_func().
> 
> Does that sound ok?

we can add that, but I don’t think it is needed. Every test case is run in its own process. And since you only have one test case at the moment, don’t over engineer this.

Regards

Marcel

Re: [PATCH] unit: check cipher support and cleanup in test-storage

[James Prestwood]

Hi,

On 5/5/25 5:55 AM, Marcel Holtmann wrote:
> Hi James,
>
>>>> The __storage_decrypt API requires AES_CTR so support should be
>>>> checked before running that test. In addition storage_exit was never
>>>> being called which leaves unbalanced mlock/munlock calls.
>>>> ---
>>>> unit/test-storage.c | 11 +++++++++--
>>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/unit/test-storage.c b/unit/test-storage.c
>>>> index c40518e6..765d9967 100644
>>>> --- a/unit/test-storage.c
>>>> +++ b/unit/test-storage.c
>>>> @@ -44,12 +44,19 @@ static void test_short_encrypted_bytes(const void *data)
>>>>
>>>> int main(int argc, char *argv[])
>>>> {
>>>> + int ret;
>>>> +
>>>> l_test_init(&argc, &argv);
>>>>
>>>> storage_init((const uint8_t *)"abc123", 6);
>>>>
>>>> - l_test_add("/storage/profile encryption",
>>>> + if (l_cipher_is_supported(L_CIPHER_AES_CTR))
>>>> + l_test_add("/storage/profile encryption",
>>>> test_short_encrypted_bytes, NULL);
>>> just use L_TEST_FLAG_ALLOW_FAILURE instead.
>> Ok, new way of doing things I guess.
>>>> - return l_test_run();
>>>> + ret = l_test_run();
>>>> +
>>>> + storage_exit();
>>>> +
>>>> + return ret;
>>> I really prefer we keep “return l_test_run()” as the basics on how test case are run.
>>>
>>> Just put storage_init,storage_exit into the test case itself. I would just do system_key_set = false in the exit function.
>> I'm fine with this, but its actually not possible to perform the cleanup after a failed test. If we want to do this we need actual cleanup support in l_test. Looks like we'd need to add a destroy function to l_test_add_data_func().
>>
>> Does that sound ok?
> we can add that, but I don’t think it is needed. Every test case is run in its own process. And since you only have one test case at the moment, don’t over engineer this.
Yeah, but what I'm saying is that there is no way to perform cleanup if 
a test asserts. In this case though I actually don't think 
storage_exit() is technically needed since the OS process cleanup will 
munlock any pages. So for now I'll just add the flag and leave 
everything else as-is.
>
> Regards
>
> Marcel
>