| Message ID | 1417576809-19173-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
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@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 = >estguid2; > - 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?
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@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 = >estguid2; >> - 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?
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@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 = >estguid2; >>> - 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
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@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 = >estguid2; >>>> - 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
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 = >estguid2; - 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; }
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@canonical.com> --- src/uefi/uefirtvariable/uefirtvariable.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)