diff mbox series

cmd: env: fix unreachable statements

Message ID 20200508055145.7621-1-takahiro.akashi@linaro.org
State Rejected
Delegated to: Heinrich Schuchardt
Headers show
Series cmd: env: fix unreachable statements | expand

Commit Message

Takahiro Akashi May 8, 2020, 5:51 a.m. UTC
C's switch statement takes an integer value for switching.
As efi_status_t is defined as unsigned long and each error code has
the top bit set, all "cases" cannot be reachable.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reported-by: Coverity (CID 300335)
---
 cmd/nvedit_efi.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

Heinrich Schuchardt May 8, 2020, 4:10 p.m. UTC | #1
On 08.05.20 07:51, AKASHI Takahiro wrote:
> C's switch statement takes an integer value for switching.
> As efi_status_t is defined as unsigned long and each error code has
> the top bit set, all "cases" cannot be reachable.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reported-by: Coverity (CID 300335)

The requirement of C 1999 specification is:
"The controlling expression of a switch statement shall have integer
type." The requirement is not that the controlling expression should be int.

GCC works fine with uint64_t as argument of a switch statement.

To me this is a false positive of Coverity.

Best regards

Heinrich

> ---
>  cmd/nvedit_efi.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
> index 837e39e02179..84cba0c7324b 100644
> --- a/cmd/nvedit_efi.c
> +++ b/cmd/nvedit_efi.c
> @@ -597,26 +597,18 @@ int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	} else {
>  		const char *msg;
>
> -		switch (ret) {
> -		case EFI_NOT_FOUND:
> +		if (ret == EFI_NOT_FOUND)
>  			msg = " (not found)";
> -			break;
> -		case EFI_WRITE_PROTECTED:
> +		else if (ret == EFI_WRITE_PROTECTED)
>  			msg = " (read only)";
> -			break;
> -		case EFI_INVALID_PARAMETER:
> +		else if (ret == EFI_INVALID_PARAMETER)
>  			msg = " (invalid parameter)";
> -			break;
> -		case EFI_SECURITY_VIOLATION:
> +		else if (ret == EFI_SECURITY_VIOLATION)
>  			msg = " (validation failed)";
> -			break;
> -		case EFI_OUT_OF_RESOURCES:
> +		else if (ret == EFI_OUT_OF_RESOURCES)
>  			msg = " (out of memory)";
> -			break;
> -		default:
> +		else
>  			msg = "";
> -			break;
> -		}
>  		printf("## Failed to set EFI variable%s\n", msg);
>  		ret = CMD_RET_FAILURE;
>  	}
>
Tom Rini May 8, 2020, 4:19 p.m. UTC | #2
On Fri, May 08, 2020 at 06:10:27PM +0200, Heinrich Schuchardt wrote:
> On 08.05.20 07:51, AKASHI Takahiro wrote:
> > C's switch statement takes an integer value for switching.
> > As efi_status_t is defined as unsigned long and each error code has
> > the top bit set, all "cases" cannot be reachable.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reported-by: Coverity (CID 300335)
> 
> The requirement of C 1999 specification is:
> "The controlling expression of a switch statement shall have integer
> type." The requirement is not that the controlling expression should be int.
> 
> GCC works fine with uint64_t as argument of a switch statement.

OK, but for the record what about C11 with GNU extensions?

> To me this is a false positive of Coverity.

I can go mark it as such after the above is answered, thanks!
Heinrich Schuchardt May 8, 2020, 5:27 p.m. UTC | #3
On 5/8/20 6:19 PM, Tom Rini wrote:
> On Fri, May 08, 2020 at 06:10:27PM +0200, Heinrich Schuchardt
> wrote:
>> On 08.05.20 07:51, AKASHI Takahiro wrote:
>>> C's switch statement takes an integer value for switching. As
>>> efi_status_t is defined as unsigned long and each error code
>>> has the top bit set, all "cases" cannot be reachable.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> Reported-by: Coverity (CID 300335)
>>
>> The requirement of C 1999 specification is: "The controlling
>> expression of a switch statement shall have integer type." The
>> requirement is not that the controlling expression should be
>> int.
>>
>> GCC works fine with uint64_t as argument of a switch statement.
>
> OK, but for the record what about C11 with GNU extensions?

C11: "The controlling expression of a switchstatement shall have
integer type." (https://web.cs.dal.ca/~vlado/pl/C_Standard_2011-n1570.pdf)

Why did you expect anything that is incompatible to C99?

Best regards

Heinrich

>
>> To me this is a false positive of Coverity.
>
> I can go mark it as such after the above is answered, thanks!
>
Tom Rini May 8, 2020, 5:41 p.m. UTC | #4
On Fri, May 08, 2020 at 07:27:05PM +0200, Heinrich Schuchardt wrote:
> On 5/8/20 6:19 PM, Tom Rini wrote:
> > On Fri, May 08, 2020 at 06:10:27PM +0200, Heinrich Schuchardt
> > wrote:
> >> On 08.05.20 07:51, AKASHI Takahiro wrote:
> >>> C's switch statement takes an integer value for switching. As
> >>> efi_status_t is defined as unsigned long and each error code
> >>> has the top bit set, all "cases" cannot be reachable.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> Reported-by: Coverity (CID 300335)
> >>
> >> The requirement of C 1999 specification is: "The controlling
> >> expression of a switch statement shall have integer type." The
> >> requirement is not that the controlling expression should be
> >> int.
> >>
> >> GCC works fine with uint64_t as argument of a switch statement.
> >
> > OK, but for the record what about C11 with GNU extensions?
> 
> C11: "The controlling expression of a switchstatement shall have
> integer type." (https://web.cs.dal.ca/~vlado/pl/C_Standard_2011-n1570.pdf)
> 
> Why did you expect anything that is incompatible to C99?

I didn't really, I just wanted a reference to C11.  It's odd I think
Coverity hit this as a false positive, but it's just a tool.  Thanks!
diff mbox series

Patch

diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
index 837e39e02179..84cba0c7324b 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -597,26 +597,18 @@  int do_env_set_efi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else {
 		const char *msg;
 
-		switch (ret) {
-		case EFI_NOT_FOUND:
+		if (ret == EFI_NOT_FOUND)
 			msg = " (not found)";
-			break;
-		case EFI_WRITE_PROTECTED:
+		else if (ret == EFI_WRITE_PROTECTED)
 			msg = " (read only)";
-			break;
-		case EFI_INVALID_PARAMETER:
+		else if (ret == EFI_INVALID_PARAMETER)
 			msg = " (invalid parameter)";
-			break;
-		case EFI_SECURITY_VIOLATION:
+		else if (ret == EFI_SECURITY_VIOLATION)
 			msg = " (validation failed)";
-			break;
-		case EFI_OUT_OF_RESOURCES:
+		else if (ret == EFI_OUT_OF_RESOURCES)
 			msg = " (out of memory)";
-			break;
-		default:
+		else
 			msg = "";
-			break;
-		}
 		printf("## Failed to set EFI variable%s\n", msg);
 		ret = CMD_RET_FAILURE;
 	}