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

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
>