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

[ivanhu]

On 2014年12月03日 17:39, Colin Ian King wrote:
> 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
>
>
>
Thanks! Colin,
will sent out another patch.

Ivan