Message ID | 3ec419e30227a8016c28e04524cd36a549aaddcf.1709898466.git.federico.serafini@bugseng.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [XEN] xen/evtchn: address violations of MISRA C:2012 Rules 16.3 and 16.4 | expand |
On Fri, 8 Mar 2024, Federico Serafini wrote: > Add missing break statements to address violations of MISRA C:2012 > Rule 16.3 ("An unconditional `break' statement shall terminate every > switch-clause"). > > Add missing default cases to address violations of MISRA C:2012 > Rule 16.4 (Every `switch' statement shall have a `default' label). > > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 08.03.2024 12:51, Federico Serafini wrote: > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq) > > case VIRQ_ARCH_0 ... VIRQ_ARCH_7: > return arch_virq_is_global(virq); > + > + default: > + ASSERT(virq < NR_VIRQS); > + break; > } > > - ASSERT(virq < NR_VIRQS); > return true; > } Just for my understanding: The ASSERT() is moved so the "default" would consist of more than just "break". Why is it that then the "return" isn't moved, too? > @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport) > break; > default: > ret = -EINVAL; > + break; > } I certainly agree here. > @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d) > case ECS_VIRQ: > printk(" v=%d", chn->u.virq); > break; > + default: > + /* Nothing to do in other cases. */ > + break; > } Yes this, just to mention it, while in line with what Misra demands is pretty meaningless imo: The absence of "default" says exactly what the comment now says. FTAOD - this is a comment towards the Misra guideline, not so much towards the specific change here. One other remark though, considering the specific function we're in: In a certifiable environment, will there actually be the capability to issue debug keys? Shouldn't we have a Kconfig option allowing to remove all that from a build (and then also from relevant scans)? Jan
On 11/03/24 08:40, Jan Beulich wrote: > On 08.03.2024 12:51, Federico Serafini wrote: >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq) >> >> case VIRQ_ARCH_0 ... VIRQ_ARCH_7: >> return arch_virq_is_global(virq); >> + >> + default: >> + ASSERT(virq < NR_VIRQS); >> + break; >> } >> >> - ASSERT(virq < NR_VIRQS); >> return true; >> } > > Just for my understanding: The ASSERT() is moved so the "default" would > consist of more than just "break". Why is it that then the "return" isn't > moved, too? No reason in particular. If preferred, I can move it too. > >> @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport) >> break; >> default: >> ret = -EINVAL; >> + break; >> } > > I certainly agree here. > >> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d) >> case ECS_VIRQ: >> printk(" v=%d", chn->u.virq); >> break; >> + default: >> + /* Nothing to do in other cases. */ >> + break; >> } > > Yes this, just to mention it, while in line with what Misra demands is > pretty meaningless imo: The absence of "default" says exactly what the > comment now says. FTAOD - this is a comment towards the Misra guideline, > not so much towards the specific change here. Both you and Stefano reviewed the code and agreed on the fact that doing nothing for the default case is the right thing and now the code explicitly says that without letting any doubts. Furthermore, during the reviews it could happen that you notice a switch where something needs to be done for the default case.
On 11.03.2024 10:02, Federico Serafini wrote: > On 11/03/24 08:40, Jan Beulich wrote: >> On 08.03.2024 12:51, Federico Serafini wrote: >>> --- a/xen/common/event_channel.c >>> +++ b/xen/common/event_channel.c >>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq) >>> >>> case VIRQ_ARCH_0 ... VIRQ_ARCH_7: >>> return arch_virq_is_global(virq); >>> + >>> + default: >>> + ASSERT(virq < NR_VIRQS); >>> + break; >>> } >>> >>> - ASSERT(virq < NR_VIRQS); >>> return true; >>> } >> >> Just for my understanding: The ASSERT() is moved so the "default" would >> consist of more than just "break". Why is it that then the "return" isn't >> moved, too? > > No reason in particular. > If preferred, I can move it too. I for one would prefer that, yes. But what's needed up front is that we decide what we want to do _consistently_ in all such cases. >>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d) >>> case ECS_VIRQ: >>> printk(" v=%d", chn->u.virq); >>> break; >>> + default: >>> + /* Nothing to do in other cases. */ >>> + break; >>> } >> >> Yes this, just to mention it, while in line with what Misra demands is >> pretty meaningless imo: The absence of "default" says exactly what the >> comment now says. FTAOD - this is a comment towards the Misra guideline, >> not so much towards the specific change here. > > Both you and Stefano reviewed the code and agreed on the fact that doing > nothing for the default case is the right thing and now the code > explicitly says that without letting any doubts. > Furthermore, during the reviews it could happen that you notice a switch > where something needs to be done for the default case. That shouldn't happen during review. Anyone proposing a patch to add such a comment wants to first have made sure the comment is actually applicable there. Otherwise we're in "mechanically add comments" territory, which I think we all agreed we want to avoid. Jan
On 11/03/24 10:51, Jan Beulich wrote: > On 11.03.2024 10:02, Federico Serafini wrote: >> On 11/03/24 08:40, Jan Beulich wrote: >>> On 08.03.2024 12:51, Federico Serafini wrote: >>>> --- a/xen/common/event_channel.c >>>> +++ b/xen/common/event_channel.c >>>> @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq) >>>> >>>> case VIRQ_ARCH_0 ... VIRQ_ARCH_7: >>>> return arch_virq_is_global(virq); >>>> + >>>> + default: >>>> + ASSERT(virq < NR_VIRQS); >>>> + break; >>>> } >>>> >>>> - ASSERT(virq < NR_VIRQS); >>>> return true; >>>> } >>> >>> Just for my understanding: The ASSERT() is moved so the "default" would >>> consist of more than just "break". Why is it that then the "return" isn't >>> moved, too? >> >> No reason in particular. >> If preferred, I can move it too. > > I for one would prefer that, yes. But what's needed up front is that we > decide what we want to do _consistently_ in all such cases. The only reason I left the return at the end is because it is slightly more compliant to advisory (and not accepted) Rule 15.5 that says to place a single return at the end of the function and to not use early returns. > >>>> @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d) >>>> case ECS_VIRQ: >>>> printk(" v=%d", chn->u.virq); >>>> break; >>>> + default: >>>> + /* Nothing to do in other cases. */ >>>> + break; >>>> } >>> >>> Yes this, just to mention it, while in line with what Misra demands is >>> pretty meaningless imo: The absence of "default" says exactly what the >>> comment now says. FTAOD - this is a comment towards the Misra guideline, >>> not so much towards the specific change here. >> >> Both you and Stefano reviewed the code and agreed on the fact that doing >> nothing for the default case is the right thing and now the code >> explicitly says that without letting any doubts. >> Furthermore, during the reviews it could happen that you notice a switch >> where something needs to be done for the default case. > > That shouldn't happen during review. Anyone proposing a patch to add such > a comment wants to first have made sure the comment is actually applicable > there. Otherwise we're in "mechanically add comments" territory, which I > think we all agreed we want to avoid. What I wanted to say is that adding the comment is not completely meaningless: it makes the intention of the code explicit and such intention is double/triple checked.
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 15aec5dcbb..cf19020e49 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -130,9 +130,12 @@ static bool virq_is_global(unsigned int virq) case VIRQ_ARCH_0 ... VIRQ_ARCH_7: return arch_virq_is_global(virq); + + default: + ASSERT(virq < NR_VIRQS); + break; } - ASSERT(virq < NR_VIRQS); return true; } @@ -846,6 +849,7 @@ int evtchn_send(struct domain *ld, unsigned int lport) break; default: ret = -EINVAL; + break; } out: @@ -1672,6 +1676,9 @@ static void domain_dump_evtchn_info(struct domain *d) case ECS_VIRQ: printk(" v=%d", chn->u.virq); break; + default: + /* Nothing to do in other cases. */ + break; } ssid = xsm_show_security_evtchn(d, chn);
Add missing break statements to address violations of MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall terminate every switch-clause"). Add missing default cases to address violations of MISRA C:2012 Rule 16.4 (Every `switch' statement shall have a `default' label). No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/common/event_channel.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)