| Message ID | 5913d8871ff6c4f320c521e50e550a64e58d4351.1702283415.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 2.1 | expand |
On Mon, 11 Dec 2023, Nicola Vetrini wrote: > The "return 0" after the swich statement in 'xen/arch/x86/mm.c' > is unreachable because all switch clauses end with returns. > However, some of them can be substituted with "break"s to allow > the "return 0" outside the switch to be reachable. > > No functional changes. This is correct but makes the code inconsistent. I would either remove the return 0; at the end of arch_memory_op, or do the following: - initialize rc to 0 at the beginning: int rc = 0; - all switch clauses break instead of return; - at the end: return rc; > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/arch/x86/mm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 0a66db10b959..8b31426a5348 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4753,7 +4753,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > } > > spin_unlock(&d->arch.e820_lock); > - return 0; > + break; > } > > case XENMEM_machine_memory_map: > @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( __copy_to_guest(arg, &ctxt.map, 1) ) > return -EFAULT; > > - return 0; > + break; > } > > case XENMEM_machphys_mapping: > @@ -4834,7 +4834,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > if ( copy_to_guest(arg, &mapping, 1) ) > return -EFAULT; > > - return 0; > + break; > } > > #ifdef CONFIG_HVM > -- > 2.34.1 > >
On 2023-12-12 02:42, Stefano Stabellini wrote: > On Mon, 11 Dec 2023, Nicola Vetrini wrote: >> The "return 0" after the swich statement in 'xen/arch/x86/mm.c' >> is unreachable because all switch clauses end with returns. >> However, some of them can be substituted with "break"s to allow >> the "return 0" outside the switch to be reachable. >> >> No functional changes. > > This is correct but makes the code inconsistent. I would either remove > the return 0; at the end of arch_memory_op, or do the following: > > - initialize rc to 0 at the beginning: int rc = 0; > - all switch clauses break instead of return; > - at the end: return rc; > Given the feedback on the Arm side, the first solution is likely to be preferred.
On 12.12.2023 10:12, Nicola Vetrini wrote: > On 2023-12-12 02:42, Stefano Stabellini wrote: >> On Mon, 11 Dec 2023, Nicola Vetrini wrote: >>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c' >>> is unreachable because all switch clauses end with returns. >>> However, some of them can be substituted with "break"s to allow >>> the "return 0" outside the switch to be reachable. >>> >>> No functional changes. >> >> This is correct but makes the code inconsistent. I would either remove >> the return 0; at the end of arch_memory_op, or do the following: >> >> - initialize rc to 0 at the beginning: int rc = 0; >> - all switch clauses break instead of return; >> - at the end: return rc; > > Given the feedback on the Arm side, the first solution is likely to be > preferred. I wouldn't mind either option, with - the former ensured to be okay with all compiler versions we (still) support, - the latter having the initialize rc to 0 part dropped; imo it's better if every case block makes sure to set the intended value explicitly. Jan
On 2023-12-12 10:53, Jan Beulich wrote: > On 12.12.2023 10:12, Nicola Vetrini wrote: >> On 2023-12-12 02:42, Stefano Stabellini wrote: >>> On Mon, 11 Dec 2023, Nicola Vetrini wrote: >>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c' >>>> is unreachable because all switch clauses end with returns. >>>> However, some of them can be substituted with "break"s to allow >>>> the "return 0" outside the switch to be reachable. >>>> >>>> No functional changes. >>> >>> This is correct but makes the code inconsistent. I would either >>> remove >>> the return 0; at the end of arch_memory_op, or do the following: >>> >>> - initialize rc to 0 at the beginning: int rc = 0; >>> - all switch clauses break instead of return; >>> - at the end: return rc; >> >> Given the feedback on the Arm side, the first solution is likely to be >> preferred. > > I wouldn't mind either option, with > - the former ensured to be okay with all compiler versions we (still) > support, I tested a stripped-down version of the switch on godbolt.org (as far back as gcc-4.8.5) and it doesn't complain. It should be tested on a real Xen build, though. > - the latter having the initialize rc to 0 part dropped; imo it's > better > if every case block makes sure to set the intended value explicitly. > This is a lot of churn, I'd rather avoid it.
On 13.12.2023 15:44, Nicola Vetrini wrote: > On 2023-12-12 10:53, Jan Beulich wrote: >> On 12.12.2023 10:12, Nicola Vetrini wrote: >>> On 2023-12-12 02:42, Stefano Stabellini wrote: >>>> On Mon, 11 Dec 2023, Nicola Vetrini wrote: >>>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c' >>>>> is unreachable because all switch clauses end with returns. >>>>> However, some of them can be substituted with "break"s to allow >>>>> the "return 0" outside the switch to be reachable. >>>>> >>>>> No functional changes. >>>> >>>> This is correct but makes the code inconsistent. I would either >>>> remove >>>> the return 0; at the end of arch_memory_op, or do the following: >>>> >>>> - initialize rc to 0 at the beginning: int rc = 0; >>>> - all switch clauses break instead of return; >>>> - at the end: return rc; >>> >>> Given the feedback on the Arm side, the first solution is likely to be >>> preferred. >> >> I wouldn't mind either option, with >> - the former ensured to be okay with all compiler versions we (still) >> support, > > I tested a stripped-down version of the switch on godbolt.org (as far > back as gcc-4.8.5) and it doesn't complain. It should be tested on a > real Xen build, though. I didn't fear any issue when going back to just 4.8. Quoting ./README: - For x86: - GCC 4.1.2_20070115 or later >> - the latter having the initialize rc to 0 part dropped; imo it's >> better >> if every case block makes sure to set the intended value explicitly. > > This is a lot of churn, I'd rather avoid it. Rant (sorry): There's already excessive churn for entirely benign issues that Misra claims need adjusting. Jan
On 2023-12-14 08:57, Jan Beulich wrote: > On 13.12.2023 15:44, Nicola Vetrini wrote: >> On 2023-12-12 10:53, Jan Beulich wrote: >>> On 12.12.2023 10:12, Nicola Vetrini wrote: >>>> On 2023-12-12 02:42, Stefano Stabellini wrote: >>>>> On Mon, 11 Dec 2023, Nicola Vetrini wrote: >>>>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c' >>>>>> is unreachable because all switch clauses end with returns. >>>>>> However, some of them can be substituted with "break"s to allow >>>>>> the "return 0" outside the switch to be reachable. >>>>>> >>>>>> No functional changes. >>>>> >>>>> This is correct but makes the code inconsistent. I would either >>>>> remove >>>>> the return 0; at the end of arch_memory_op, or do the following: >>>>> >>>>> - initialize rc to 0 at the beginning: int rc = 0; >>>>> - all switch clauses break instead of return; >>>>> - at the end: return rc; >>>> >>>> Given the feedback on the Arm side, the first solution is likely to >>>> be >>>> preferred. >>> >>> I wouldn't mind either option, with >>> - the former ensured to be okay with all compiler versions we (still) >>> support, >> >> I tested a stripped-down version of the switch on godbolt.org (as far >> back as gcc-4.8.5) and it doesn't complain. It should be tested on a >> real Xen build, though. > > I didn't fear any issue when going back to just 4.8. Quoting ./README: > > - For x86: > - GCC 4.1.2_20070115 or later > I found no issue in 4.1.2 (see https://godbolt.org/z/cxecnKseG) >>> - the latter having the initialize rc to 0 part dropped; imo it's >>> better >>> if every case block makes sure to set the intended value >>> explicitly. >> >> This is a lot of churn, I'd rather avoid it. > > Rant (sorry): There's already excessive churn for entirely benign > issues > that Misra claims need adjusting. > > Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 0a66db10b959..8b31426a5348 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4753,7 +4753,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) } spin_unlock(&d->arch.e820_lock); - return 0; + break; } case XENMEM_machine_memory_map: @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( __copy_to_guest(arg, &ctxt.map, 1) ) return -EFAULT; - return 0; + break; } case XENMEM_machphys_mapping: @@ -4834,7 +4834,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( copy_to_guest(arg, &mapping, 1) ) return -EFAULT; - return 0; + break; } #ifdef CONFIG_HVM
The "return 0" after the swich statement in 'xen/arch/x86/mm.c' is unreachable because all switch clauses end with returns. However, some of them can be substituted with "break"s to allow the "return 0" outside the switch to be reachable. No functional changes. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/mm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)