[PATCH] uefirtvariable: check the return value for cleaning up the enviroment

[Colin Ian King]

On 03/12/14 09:36, ivanhu wrote:
> 
> On 2014年12月03日 15:22, Alex Hung wrote:
>> On 12/03/2014 11:20 AM, Ivan Hu wrote:
>>> Coverity Scan complains the Unchecked return value for ioctl() calls in
>>> uefirtvariable_env_cleanup(). The uefirtvariable_env_cleanup() is
>>> added to avoid
>>> some buggy firmware abnormal stop the tests without deleted the test
>>> variables.
>>> Before starting the test, it will try to delete all the test
>>> variables. Expected
>>> return -1 and errno EINVAL when no test variables exist, return 0
>>> when there are
>>> test variables and deleted sucessfully. Check the return value and
>>> log info
>>> when other errors occur.
>>>
>>> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
>>> ---
>>>   src/uefi/uefirtvariable/uefirtvariable.c |   26
>>> ++++++++++++++++++++------
>>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c
>>> b/src/uefi/uefirtvariable/uefirtvariable.c
>>> index a19f835..3bb5ca0 100644
>>> --- a/src/uefi/uefirtvariable/uefirtvariable.c
>>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>>> @@ -21,6 +21,7 @@
>>>   #include <stdio.h>
>>>   #include <stddef.h>
>>>   #include <sys/ioctl.h>
>>> +#include <errno.h>
>>>   #include <fcntl.h>
>>>     #include "fwts.h"
>>> @@ -62,8 +63,9 @@ 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)
>>> +static void uefirtvariable_env_cleanup(fwts_framework *fw)
>>>   {
>>> +    long ioret;
>>>       struct efi_setvariable setvariable;
>>>       uint64_t status;
>>>       uint8_t data = 0;
>>> @@ -74,17 +76,29 @@ static void uefirtvariable_env_cleanup(void)
>>>       setvariable.DataSize = 0;
>>>       setvariable.Data = &data;
>>>       setvariable.status = &status;
>>> -    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    if (ioret == -1 && errno != EINVAL)
>>> +        fwts_log_info(fw, "Got errors when clean up the enviroment, "
>>> +                    "but still try to run the tests.");
>>>         setvariable.VariableName = variablenametest2;
>>> -    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    if (ioret == -1 && errno != EINVAL)
>>> +        fwts_log_info(fw, "Got errors when clean up the enviroment, "
>>> +                    "but still try to run the tests.");
>>>         setvariable.VariableName = variablenametest3;
>>> -    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    if (ioret == -1 && errno != EINVAL)
>>> +        fwts_log_info(fw, "Got errors when clean up the enviroment, "
>>> +                    "but still try to run the tests.");
>>>         setvariable.VariableName = variablenametest;
>>>       setvariable.VendorGuid = &gtestguid2;
>>> -    ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>> +    if (ioret == -1 && errno != EINVAL)
>>> +        fwts_log_info(fw, "Got errors when clean up the enviroment, "
>>> +                    "but still try to run the tests.");
>>>   }
>>>     static int uefirtvariable_init(fwts_framework *fw)
>>> @@ -105,7 +119,7 @@ static int uefirtvariable_init(fwts_framework *fw)
>>>           return FWTS_ABORTED;
>>>       }
>>>   -    uefirtvariable_env_cleanup();
>>> +    uefirtvariable_env_cleanup(fw);
>>>         return FWTS_OK;
>>>   }
>>>
>> Since errors are returned, will it be better if warnings are used
>> instead?
>>
> The return value of the uefirtvariable_env_cleanup() doesn't need to be
> cared.
> In normal case, without uefirtvariable_env_cleanup() the test also run
> well.
> These check value codes are added to fix the Coverity Scan warnings.
> Most case on fwts the warnings will accompany with FWTS_ERROR. I prefer
> log info rather than warnings here.
> Any one has comments?
> 
> 
> 
If one does not care about checking ioctl() error returns then one could
just explicitly cast the return to void, e.g.

	(void)ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);

Note sure how well static analysers handle that. Try than and if
coverity complains I will handle the exceptions in coverity scan

Colin