| Message ID | acb26329a980809dda100825f52b05d0cc295315.1719383180.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 26.06.2024 11:28, 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> Tentatively Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- 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 -EPERM; > } > } > else if ( l1f & _PAGE_RW ) I don't like the use of -EPERM here very much, but I understand that there's no really suitable errno value. I wonder though whether something far more "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL. Just to mention it: -EPERM is what failed XSM checks would typically yield, so from that perspective alone even switching to -EACCES might be a little bit better. I further wonder whether, with the assertion catching an issue with the implementation, we shouldn't consider using BUG() here instead. Input from in particular the other x86 maintainers appreciated. Jan
On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote: > On 26.06.2024 11:28, 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> > > Tentatively > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > --- 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 -EPERM; > > } > > } > > else if ( l1f & _PAGE_RW ) > > I don't like the use of -EPERM here very much, but I understand that there's > no really suitable errno value. I wonder though whether something far more > "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL. > Just to mention it: -EPERM is what failed XSM checks would typically yield, > so from that perspective alone even switching to -EACCES might be a little > bit better. > fwiw: EACCES, being typically used for interface version mismatches, would confuse me a lot. > I further wonder whether, with the assertion catching an issue with the > implementation, we shouldn't consider using BUG() here instead. Input from > in particular the other x86 maintainers appreciated. > > Jan Cheers, Alejandro
On 09.07.2024 13:21, Alejandro Vallejo wrote: > On Mon Jul 1, 2024 at 9:57 AM BST, Jan Beulich wrote: >> On 26.06.2024 11:28, Federico Serafini wrote: >>> --- 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 -EPERM; >>> } >>> } >>> else if ( l1f & _PAGE_RW ) >> >> I don't like the use of -EPERM here very much, but I understand that there's >> no really suitable errno value. I wonder though whether something far more >> "exotic" wouldn't be better in such a case, say -EBADMSG or -EADDRNOTAVAIL. >> Just to mention it: -EPERM is what failed XSM checks would typically yield, >> so from that perspective alone even switching to -EACCES might be a little >> bit better. > > fwiw: EACCES, being typically used for interface version mismatches, would > confuse me a lot. There's no interface version check anywhere in hypercalls involving get_page_from_l1e(), I don't think. So I see little room for confusion. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 648d6dd475..a1e28b3360 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 -EPERM; } } 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> --- Changes in v3: - do not return 0 (success). Al least this version returns an error code but I am not sure about which one to use. --- xen/arch/x86/mm.c | 1 + 1 file changed, 1 insertion(+)