| Message ID | f3adf3d5dac5c4c108207883472f3ebcfa11c685.1719218291.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address some violations of MISRA C Rule 16.3 | expand |
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
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
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.
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 )
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(+)