[v2] x86: address some violations of MISRA C Rule 16.3

[XEN PATCH v2 09/13] x86/mm: add defensive return

Posted by Federico Serafini 5 months ago

Add defensive return statement at the end of an unreachable
default case. Other than improve safety, this meets the requirements
to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
statement shall terminate every switch-clause".

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/mm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 648d6dd475..2e19ced15e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -916,6 +916,7 @@ get_page_from_l1e(
                 return 0;
             default:
                 ASSERT_UNREACHABLE();
+                return 0;
             }
         }
         else if ( l1f & _PAGE_RW )
-- 
2.34.1

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

Posted by Stefano Stabellini 5 months ago

On Mon, 24 Jun 2024, Federico Serafini wrote:
> Add defensive return statement at the end of an unreachable
> default case. Other than improve safety, this meets the requirements
> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/mm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 648d6dd475..2e19ced15e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -916,6 +916,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return 0;

Let's avoid this

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

Posted by Jan Beulich 5 months ago

On 24.06.2024 11:04, Federico Serafini wrote:
> Add defensive return statement at the end of an unreachable
> default case. Other than improve safety,

It is particularly with this in mind that ...

> this meets the requirements
> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
> statement shall terminate every switch-clause".
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/mm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 648d6dd475..2e19ced15e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -916,6 +916,7 @@ get_page_from_l1e(
>                  return 0;
>              default:
>                  ASSERT_UNREACHABLE();
> +                return 0;
>              }

... returning "success" in such a case can't be quite right. As indicated
elsewhere, really we want -EINTERNAL for such cases, just that errno.h
doesn't know anything like this, and I'm also unaware of a somewhat
similar identifier that we might "clone" from elsewhere. Hence we'll want
to settle on some existing error code which we then use here and in
similar situations. EINVAL is the primary example of what I would prefer
to _not_ see used for this.

Jan

Re: [XEN PATCH v2 09/13] x86/mm: add defensive return

Posted by Federico Serafini 5 months ago

On 24/06/24 17:39, Jan Beulich wrote:
> On 24.06.2024 11:04, Federico Serafini wrote:
>> Add defensive return statement at the end of an unreachable
>> default case. Other than improve safety,
> 
> It is particularly with this in mind that ...
> 
>> this meets the requirements
>> to deviate a violation of MISRA C Rule 16.3: "An unconditional `break'
>> statement shall terminate every switch-clause".
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>> ---
>>   xen/arch/x86/mm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 648d6dd475..2e19ced15e 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -916,6 +916,7 @@ get_page_from_l1e(
>>                   return 0;
>>               default:
>>                   ASSERT_UNREACHABLE();
>> +                return 0;
>>               }
> 
> ... returning "success" in such a case can't be quite right. As indicated
> elsewhere, really we want -EINTERNAL for such cases, just that errno.h
> doesn't know anything like this, and I'm also unaware of a somewhat
> similar identifier that we might "clone" from elsewhere. Hence we'll want
> to settle on some existing error code which we then use here and in
> similar situations. EINVAL is the primary example of what I would prefer
> to _not_ see used for this.

Sorry, I got confused by the ASSERT_UNREACHABLE followed by a
return 0 few lines above.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)