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

[ivanhu]

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?