ACK: [PATCH 2/2] fwts: uefi: clean up all test variable before doing the uefirtvariable test (LP: #1313554)

[Colin Ian King]

On 02/05/14 15:49, IvanHu wrote:
> On 05/02/2014 05:44 PM, Colin Ian King wrote:
>> On 28/04/14 09:07, Ivan Hu wrote:
>>> Try to delete all the test variables that used by the uefirtvariable
>>> test,
>>> before doing the uefirtvariable tests. Make sure there are no
>>> preexisting
>>> test variables that cause the the uefirtvariable test fail, when the
>>> variable tests accidentally stopped, terminated by the buggy
>>> firmware, power
>>> fail etc.
>>>
>>> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
>>> ---
>>>   src/uefi/uefirtvariable/uefirtvariable.c |   31
>>> ++++++++++++++++++++++++++++--
>>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c
>>> b/src/uefi/uefirtvariable/uefirtvariable.c
>>> index 6e40542..7ddfeb1 100644
>>> --- a/src/uefi/uefirtvariable/uefirtvariable.c
>>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>>> @@ -59,6 +59,33 @@ static uint32_t attributes =
>>>       FWTS_UEFI_VAR_BOOTSERVICE_ACCESS |
>>>       FWTS_UEFI_VAR_RUNTIME_ACCESS;
>>>   static uint16_t variablenametest[] = {'T', 'e', 's', 't', 'v', 'a',
>>> 'r', '\0'};
>>> +static uint16_t variablenametest2[] = {'T', 'e', 's', 't', 'v', 'a',
>>> 'r', ' ', '\0'};
>>> +static uint16_t variablenametest3[] = {'T', 'e', 's', 't', 'v', 'a',
>>> '\0'};
>>> +
>>> +static void uefirtvariable_env_cleanup(void)
>>> +{
>>> +    struct efi_setvariable setvariable;
>>> +    uint64_t status;
>>> +    uint8_t data = 0;
>>> +
>>> +    setvariable.VariableName = variablenametest;
>>> +    setvariable.VendorGuid = &gtestguid1;
>>> +    setvariable.Attributes = 0;
>>> +    setvariable.DataSize = 0;
>>> +    setvariable.Data = &data;
>>> +    setvariable.status = &status;
>>> +    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +
>>> +    setvariable.VariableName = variablenametest2;
>>> +    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +
>>> +    setvariable.VariableName = variablenametest3;
>>> +    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +
>>> +    setvariable.VariableName = variablenametest;
>>> +    setvariable.VendorGuid = &gtestguid2;
>>> +    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +}
>>
>> I see that we're not checking the ioctl return here.  What is the
>> outcome if the variables do exist and the deletion fails to the tests
>> that require these variables to not exist?
> 
> Actually, I expect that the test variables do not exist, I used the
> unique GUID, and name, and after each tests the test variables will be
> deleted. I delete the test variables without checking the result,
> because I don't care if the variables are deleted or not found. Adding
> the deleting code just for double confirm that the test variables do not
> exist. Because like the bug#1310686, back-to-back runs has different
> results issue,
> https://bugs.launchpad.net/fwts/+bug/1310686
> the buggy firmware set variable successfully with invalid attribute, but
> cannot be found by get variable, so that test variable doesn't be
> deleted. Although, another patch "uefi: uefirtvariable: report failure
> when setting variable with invalid attribute successfully" fixed this
> issue. But the machine has polluted, the test variable existed in that
> machine, it cannot be tested any more before manually delete the test
> variables, setvariable will return EFI_INVALID_PARAMETER. So I think it
> might be good idea to try to delete the test variable each time we would
> like to do the variable test.
> 
>>
>> Perhaps a more "robust" thing to do is:
>>
>> Check if they exist, *then* attempt to deleted them, and if that fails,
>> report this as an error as this will mess up subsequent tests.
> 
> Like the above example, the test variable do exist, but it cannot be
> found by getvariable on runtime. So it will not be deleted, and the
> problem is still there.

Fair enough,  in which case, I think that's an ACK from me.

Acked-by: Colin Ian King <colin.king at canonical.com>
> 
>>
>> Or am I being overly pedantic here?
> 
> 
> 
>>
>>>
>>>   static int uefirtvariable_init(fwts_framework *fw)
>>>   {
>>> @@ -78,6 +105,8 @@ static int uefirtvariable_init(fwts_framework *fw)
>>>           return FWTS_ABORTED;
>>>       }
>>>
>>> +    uefirtvariable_env_cleanup();
>>> +
>>>       return FWTS_OK;
>>>   }
>>>
>>> @@ -1044,8 +1073,6 @@ static int setvariable_test3(fwts_framework *fw)
>>>       int ret;
>>>       uint64_t datasize = 10;
>>>       uint8_t datadiff1 = 0, datadiff2 = 1, datadiff3 = 2;
>>> -    uint16_t variablenametest2[] = {'T', 'e', 's', 't', 'v', 'a',
>>> 'r', ' ', '\0'};
>>> -    uint16_t variablenametest3[] = {'T', 'e', 's', 't', 'v', 'a',
>>> '\0'};
>>>
>>>       ret = setvariable_insertvariable(fw, attributes, datasize,
>>>           variablenametest2, &gtestguid1, datadiff2);
>>>
>>
>>
> 
>