| Message ID | 24501d2e7f5d82d8e5a483a80b35e04ce765a7cf.1722239813.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: address some violations of MISRA C Rule 16.3 | expand |
On Mon, 29 Jul 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> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > No changes from v3 and v4, further feedback on this thread would be appreciated: > https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html > --- > 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 95795567f2..8973e76dff 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -917,6 +917,7 @@ get_page_from_l1e( > return 0; > default: > ASSERT_UNREACHABLE(); > + return -EPERM; > } > } > else if ( l1f & _PAGE_RW ) > -- > 2.34.1 > >
On Mon, 29 Jul 2024, Stefano Stabellini wrote: > On Mon, 29 Jul 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> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > > --- > > No changes from v3 and v4, further feedback on this thread would be appreciated: > > https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html Looking at the older threads, I looks like Jan suggested EACCES, I also think it is marginally better. My R-b stands also for EACCES. Jan, I think you should go ahead and fix on commit > > --- > > 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 95795567f2..8973e76dff 100644 > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -917,6 +917,7 @@ get_page_from_l1e( > > return 0; > > default: > > ASSERT_UNREACHABLE(); > > + return -EPERM; > > } > > } > > else if ( l1f & _PAGE_RW ) > > -- > > 2.34.1 > > > > >
On 29.08.2024 02:35, Stefano Stabellini wrote: > On Mon, 29 Jul 2024, Stefano Stabellini wrote: >> On Mon, 29 Jul 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> >> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >>> --- >>> No changes from v3 and v4, further feedback on this thread would be appreciated: >>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html > > Looking at the older threads, I looks like Jan suggested EACCES, I also > think it is marginally better. My R-b stands also for EACCES. Jan, I > think you should go ahead and fix on commit No, I very definitely want a 2nd x86 maintainer opinion here. Or a better suggestion for the error code to use by anyone. After all, as you confirm, EACCES is only marginally better. Jan
On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote: > On 29.08.2024 02:35, Stefano Stabellini wrote: > > On Mon, 29 Jul 2024, Stefano Stabellini wrote: > >> On Mon, 29 Jul 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> > >> > >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > >> > >>> --- > >>> No changes from v3 and v4, further feedback on this thread would be appreciated: > >>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html > > > > Looking at the older threads, I looks like Jan suggested EACCES, I also > > think it is marginally better. My R-b stands also for EACCES. Jan, I > > think you should go ahead and fix on commit > > No, I very definitely want a 2nd x86 maintainer opinion here. Or a better > suggestion for the error code to use by anyone. After all, as you confirm, > EACCES is only marginally better. Hm, the only alternative I could suggest is using ERANGE, to signal the value of opt_mmio_relax is out of the expected range, however that could be confusing for the callers? One benefit of using ERANGE is that there's currently no return path in get_page_from_l1e() with that error code, so it would be very easy to spot when an unexpected value of opt_mmio_relax is found. However there are no guarantees that further error paths might use that error code. Thanks, Roger.
On 30/08/2024 10:18 am, Roger Pau Monné wrote: > On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote: >> On 29.08.2024 02:35, Stefano Stabellini wrote: >>> On Mon, 29 Jul 2024, Stefano Stabellini wrote: >>>> On Mon, 29 Jul 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> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>>> --- >>>>> No changes from v3 and v4, further feedback on this thread would be appreciated: >>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html >>> Looking at the older threads, I looks like Jan suggested EACCES, I also >>> think it is marginally better. My R-b stands also for EACCES. Jan, I >>> think you should go ahead and fix on commit >> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better >> suggestion for the error code to use by anyone. After all, as you confirm, >> EACCES is only marginally better. > Hm, the only alternative I could suggest is using ERANGE, to signal > the value of opt_mmio_relax is out of the expected range, however that > could be confusing for the callers? > > One benefit of using ERANGE is that there's currently no return path > in get_page_from_l1e() with that error code, so it would be very easy > to spot when an unexpected value of opt_mmio_relax is found. However > there are no guarantees that further error paths might use that error > code. EPERM and EACCES are both very wrong here. They imply something that is simply not appropriate in this context. We use EILSEQ elsewhere for believed-impossible conditions where we need an errno of some kind. I suggest we use it here too. ~Andrew
On Fri, Aug 30, 2024 at 08:17:40PM +0100, Andrew Cooper wrote: > On 30/08/2024 10:18 am, Roger Pau Monné wrote: > > On Thu, Aug 29, 2024 at 09:04:37AM +0200, Jan Beulich wrote: > >> On 29.08.2024 02:35, Stefano Stabellini wrote: > >>> On Mon, 29 Jul 2024, Stefano Stabellini wrote: > >>>> On Mon, 29 Jul 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> > >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > >>>> > >>>>> --- > >>>>> No changes from v3 and v4, further feedback on this thread would be appreciated: > >>>>> https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html > >>> Looking at the older threads, I looks like Jan suggested EACCES, I also > >>> think it is marginally better. My R-b stands also for EACCES. Jan, I > >>> think you should go ahead and fix on commit > >> No, I very definitely want a 2nd x86 maintainer opinion here. Or a better > >> suggestion for the error code to use by anyone. After all, as you confirm, > >> EACCES is only marginally better. > > Hm, the only alternative I could suggest is using ERANGE, to signal > > the value of opt_mmio_relax is out of the expected range, however that > > could be confusing for the callers? > > > > One benefit of using ERANGE is that there's currently no return path > > in get_page_from_l1e() with that error code, so it would be very easy > > to spot when an unexpected value of opt_mmio_relax is found. However > > there are no guarantees that further error paths might use that error > > code. > > EPERM and EACCES are both very wrong here. They imply something that is > simply not appropriate in this context. > > We use EILSEQ elsewhere for believed-impossible conditions where we need > an errno of some kind. I suggest we use it here too. Fine with me. With that: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 95795567f2..8973e76dff 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -917,6 +917,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> --- No changes from v3 and v4, further feedback on this thread would be appreciated: https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00474.html --- xen/arch/x86/mm.c | 1 + 1 file changed, 1 insertion(+)