| Message ID | c0a8a12e39d688e101936d221af0f8eeefabe352.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: > Given that 'hwdom_shutdown' is a noreturn function, unreachable > breaks can be eliminated to resolve violations of Rule 2.1. > On the occasion, the type of its parameter is changed to uint8_t. > > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 11.12.2023 11:30, Nicola Vetrini wrote: > Given that 'hwdom_shutdown' is a noreturn function, unreachable > breaks can be eliminated to resolve violations of Rule 2.1. > On the occasion, the type of its parameter is changed to uint8_t. I don't particularly mind that extra change, but it's entirely unrelated here. We do such when we touch lines (or maybe adjacent lines) anyway, but here you had no need to touch either declaration or the header of the definition. > --- a/xen/common/shutdown.c > +++ b/xen/common/shutdown.c > @@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void) > } > } > > -void hwdom_shutdown(u8 reason) > +void hwdom_shutdown(uint8_t reason) > { > switch ( reason ) > { > @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason) > printk("Hardware Dom%u halted: halting machine\n", > hardware_domain->domain_id); > machine_halt(); > - break; /* not reached */ > > case SHUTDOWN_crash: > debugger_trap_immediate(); > printk("Hardware Dom%u crashed: ", hardware_domain->domain_id); > kexec_crash(CRASHREASON_HWDOM); > maybe_reboot(); > - break; /* not reached */ Given the function's name, I don't view this removal (and the similar one further down) as acceptable. The function first needs renaming so that from its name one can no longer (wrongly) infer that it may reboot or else return. Jan
On 2023-12-12 10:45, Jan Beulich wrote: > On 11.12.2023 11:30, Nicola Vetrini wrote: >> Given that 'hwdom_shutdown' is a noreturn function, unreachable >> breaks can be eliminated to resolve violations of Rule 2.1. >> On the occasion, the type of its parameter is changed to uint8_t. > > I don't particularly mind that extra change, but it's entirely > unrelated > here. We do such when we touch lines (or maybe adjacent lines) anyway, > but here you had no need to touch either declaration or the header of > the definition. > Ok. I can make this a separate patch, since I'm already touching the function >> --- a/xen/common/shutdown.c >> +++ b/xen/common/shutdown.c >> @@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void) >> } >> } >> >> -void hwdom_shutdown(u8 reason) >> +void hwdom_shutdown(uint8_t reason) >> { >> switch ( reason ) >> { >> @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason) >> printk("Hardware Dom%u halted: halting machine\n", >> hardware_domain->domain_id); >> machine_halt(); >> - break; /* not reached */ >> >> case SHUTDOWN_crash: >> debugger_trap_immediate(); >> printk("Hardware Dom%u crashed: ", >> hardware_domain->domain_id); >> kexec_crash(CRASHREASON_HWDOM); >> maybe_reboot(); >> - break; /* not reached */ > > Given the function's name, I don't view this removal (and the similar > one > further down) as acceptable. The function first needs renaming so that > from its name one can no longer (wrongly) infer that it may reboot or > else > return. > > Jan Something like "reboot_or_halt" would be okay?
On 12.12.2023 10:53, Nicola Vetrini wrote: > On 2023-12-12 10:45, Jan Beulich wrote: >> On 11.12.2023 11:30, Nicola Vetrini wrote: >>> @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason) >>> printk("Hardware Dom%u halted: halting machine\n", >>> hardware_domain->domain_id); >>> machine_halt(); >>> - break; /* not reached */ >>> >>> case SHUTDOWN_crash: >>> debugger_trap_immediate(); >>> printk("Hardware Dom%u crashed: ", >>> hardware_domain->domain_id); >>> kexec_crash(CRASHREASON_HWDOM); >>> maybe_reboot(); >>> - break; /* not reached */ >> >> Given the function's name, I don't view this removal (and the similar >> one >> further down) as acceptable. The function first needs renaming so that >> from its name one can no longer (wrongly) infer that it may reboot or >> else >> return. > > Something like "reboot_or_halt" would be okay? I think so, yes. Jan
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c index 37901a4f3391..290f90d70fe1 100644 --- a/xen/common/shutdown.c +++ b/xen/common/shutdown.c @@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void) } } -void hwdom_shutdown(u8 reason) +void hwdom_shutdown(uint8_t reason) { switch ( reason ) { @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason) printk("Hardware Dom%u halted: halting machine\n", hardware_domain->domain_id); machine_halt(); - break; /* not reached */ case SHUTDOWN_crash: debugger_trap_immediate(); printk("Hardware Dom%u crashed: ", hardware_domain->domain_id); kexec_crash(CRASHREASON_HWDOM); maybe_reboot(); - break; /* not reached */ case SHUTDOWN_reboot: printk("Hardware Dom%u shutdown: rebooting machine\n", hardware_domain->domain_id); machine_restart(0); - break; /* not reached */ case SHUTDOWN_watchdog: printk("Hardware Dom%u shutdown: watchdog rebooting machine\n", hardware_domain->domain_id); kexec_crash(CRASHREASON_WATCHDOG); machine_restart(0); - break; /* not reached */ case SHUTDOWN_soft_reset: printk("Hardware domain %d did unsupported soft reset, rebooting.\n", hardware_domain->domain_id); machine_restart(0); - break; /* not reached */ default: printk("Hardware Dom%u shutdown (unknown reason %u): ", hardware_domain->domain_id, reason); maybe_reboot(); - break; /* not reached */ } -} - +} diff --git a/xen/include/xen/shutdown.h b/xen/include/xen/shutdown.h index 668aed0be580..3537c30e0a1b 100644 --- a/xen/include/xen/shutdown.h +++ b/xen/include/xen/shutdown.h @@ -6,7 +6,7 @@ /* opt_noreboot: If true, machine will need manual reset on error. */ extern bool opt_noreboot; -void noreturn hwdom_shutdown(u8 reason); +void noreturn hwdom_shutdown(uint8_t reason); void noreturn machine_restart(unsigned int delay_millisecs); void noreturn machine_halt(void);
Given that 'hwdom_shutdown' is a noreturn function, unreachable breaks can be eliminated to resolve violations of Rule 2.1. On the occasion, the type of its parameter is changed to uint8_t. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/common/shutdown.c | 11 ++--------- xen/include/xen/shutdown.h | 2 +- 2 files changed, 3 insertions(+), 10 deletions(-)