| Message ID | 96a1b98d7831154c58d39b85071b9670de94aed0.1718617636.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] xen: add explicit comment to identify notifier patterns | expand |
On 17.06.2024 11:49, Federico Serafini wrote: > MISRA C Rule 16.4 states that every `switch' statement shall have a > `default' label" and a statement or a comment prior to the > terminating break statement. > > This patch addresses some violations of the rule related to the > "notifier pattern": a frequently-used pattern whereby only a few values > are handled by the switch statement and nothing should be done for > others (nothing to do in the default case). > > No functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> I guess I shouldn't outright NAK this, but I certainly won't ack it. This is precisely the purely mechanical change that in earlier discussions some (including me) have indicated isn't going to help safety. However, if others want to ack something purely mechanical like this, then my minimal requirement would be that somewhere it is spelled out what falls under > --- > xen/arch/arm/cpuerrata.c | 1 + > xen/arch/arm/gic.c | 1 + > xen/arch/arm/irq.c | 4 ++++ giv-v3-lpi.c has a similar instance, yet you don't adjust that. This may be because that possibly is the one where it was previously indicated that it may in fact be a mistake that the dying/dead case isn't handled, but then at the very least I'd have expected that you explicitly mention cases where the adjustment is (deliberately) not made. Jan
On 17/06/24 12:03, Jan Beulich wrote: > On 17.06.2024 11:49, Federico Serafini wrote: >> MISRA C Rule 16.4 states that every `switch' statement shall have a >> `default' label" and a statement or a comment prior to the >> terminating break statement. >> >> This patch addresses some violations of the rule related to the >> "notifier pattern": a frequently-used pattern whereby only a few values >> are handled by the switch statement and nothing should be done for >> others (nothing to do in the default case). >> >> No functional change. >> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > > I guess I shouldn't outright NAK this, but I certainly won't ack it. This > is precisely the purely mechanical change that in earlier discussions some > (including me) have indicated isn't going to help safety. However, if > others want to ack something purely mechanical like this, then my minimal > requirement would be that somewhere it is spelled out what falls under > >> --- >> xen/arch/arm/cpuerrata.c | 1 + >> xen/arch/arm/gic.c | 1 + >> xen/arch/arm/irq.c | 4 ++++ > > giv-v3-lpi.c has a similar instance, yet you don't adjust that. This may > be because that possibly is the one where it was previously indicated that > it may in fact be a mistake that the dying/dead case isn't handled, but > then at the very least I'd have expected that you explicitly mention cases > where the adjustment is (deliberately) not made. I did the changes accordingly to the instruction in docs/misra/rules.rst for Rule 16.4 and I touched only the files having an unjustified violations of the rule. The violation triggered by gic-v3-lpi.c is currently deviated with the following justification: "A switch statement with a single switch clause and no default label may be used in place of an equivalent if statement if it is considered to improve readability." However, I agree with you that also gic-v3.c should be consistent with the other.
On Mon, 17 Jun 2024, Jan Beulich wrote: > On 17.06.2024 11:49, Federico Serafini wrote: > > MISRA C Rule 16.4 states that every `switch' statement shall have a > > `default' label" and a statement or a comment prior to the > > terminating break statement. > > > > This patch addresses some violations of the rule related to the > > "notifier pattern": a frequently-used pattern whereby only a few values > > are handled by the switch statement and nothing should be done for > > others (nothing to do in the default case). > > > > No functional change. > > > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > > I guess I shouldn't outright NAK this, but I certainly won't ack it. This > is precisely the purely mechanical change that in earlier discussions some > (including me) have indicated isn't going to help safety. However, if > others want to ack something purely mechanical like this, then my minimal > requirement would be that somewhere it is spelled out what falls under I know there is a new version of this patch on xen-devel but I just wanted to add that although it is true that this patch taken on its own it does not improve safety, it does improve safety because it enables us to go down to zero violations on rule 16.4 and then make the rule 16.4 blocking in the gitlab-ci ECLAIR job.
On 21.06.2024 03:09, Stefano Stabellini wrote: > On Mon, 17 Jun 2024, Jan Beulich wrote: >> On 17.06.2024 11:49, Federico Serafini wrote: >>> MISRA C Rule 16.4 states that every `switch' statement shall have a >>> `default' label" and a statement or a comment prior to the >>> terminating break statement. >>> >>> This patch addresses some violations of the rule related to the >>> "notifier pattern": a frequently-used pattern whereby only a few values >>> are handled by the switch statement and nothing should be done for >>> others (nothing to do in the default case). >>> >>> No functional change. >>> >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >> >> I guess I shouldn't outright NAK this, but I certainly won't ack it. This >> is precisely the purely mechanical change that in earlier discussions some >> (including me) have indicated isn't going to help safety. However, if >> others want to ack something purely mechanical like this, then my minimal >> requirement would be that somewhere it is spelled out what falls under > > I know there is a new version of this patch on xen-devel but I just > wanted to add that although it is true that this patch taken on its own > it does not improve safety, it does improve safety because it enables us > to go down to zero violations on rule 16.4 and then make the rule 16.4 > blocking in the gitlab-ci ECLAIR job. And moving out of everyone's sight (unless specifically grep-ing for the pattern) that then there are a certain number of sites where we're not really sure. Jan
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c index 2b7101ea25..69c30aecd8 100644 --- a/xen/arch/arm/cpuerrata.c +++ b/xen/arch/arm/cpuerrata.c @@ -730,6 +730,7 @@ static int cpu_errata_callback(struct notifier_block *nfb, rc = enable_nonboot_cpu_caps(arm_errata); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 3eaf670fd7..dc5408a456 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -463,6 +463,7 @@ static int cpu_gic_callback(struct notifier_block *nfb, release_irq(gic_hw_ops->info->maintenance_irq, NULL); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index c60502444c..61ca6f5b87 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -127,6 +127,10 @@ static int cpu_callback(struct notifier_block *nfb, unsigned long action, printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n", cpu); break; + + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(rc); diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 1725cca649..bf7c66155d 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -1839,6 +1839,7 @@ static int cpu_virt_paging_callback(struct notifier_block *nfb, setup_virt_paging_one(NULL); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c index 87fe960330..81f91f05bb 100644 --- a/xen/arch/arm/percpu.c +++ b/xen/arch/arm/percpu.c @@ -66,6 +66,7 @@ static int cpu_percpu_callback( free_percpu_area(cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index 04e363088d..3d481e59f9 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -591,6 +591,7 @@ static int cpu_smpboot_callback(struct notifier_block *nfb, remove_cpu_sibling_map(cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index e74d30d258..27cbfae874 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -382,6 +382,7 @@ static int cpu_time_callback(struct notifier_block *nfb, deinit_timer_interrupt(); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index 70b5aeb822..a33ff64ff2 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -1194,6 +1194,7 @@ static void sanitize_its_base_reg(uint64_t *reg) r |= GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT; break; default: + /* Notifier pattern. */ break; } @@ -1206,6 +1207,7 @@ static void sanitize_its_base_reg(uint64_t *reg) r |= GIC_BASER_CACHE_RaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT; break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index 32c1b2756b..222b174bbb 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -722,6 +722,10 @@ static int cf_check cpu_callback( if ( park_offline_cpus ) cpu_bank_free(cpu); break; + + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(rc); diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c index 371dd100c7..d271102f9f 100644 --- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -238,6 +238,9 @@ static int cf_check update_clusterinfo( } FREE_CPUMASK_VAR(per_cpu(scratch_mask, cpu)); break; + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(err); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 8334ab1711..00c360cf24 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -123,6 +123,7 @@ static int cf_check cpu_callback( alternative_vcall(hvm_funcs.cpu_dead, cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c index 9793fa2316..105efa5a71 100644 --- a/xen/arch/x86/nmi.c +++ b/xen/arch/x86/nmi.c @@ -434,6 +434,7 @@ static int cf_check cpu_nmi_callback( kill_timer(&per_cpu(nmi_timer, cpu)); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c index 3205eacea6..627b56b9f3 100644 --- a/xen/arch/x86/percpu.c +++ b/xen/arch/x86/percpu.c @@ -84,6 +84,9 @@ static int cf_check cpu_percpu_callback( if ( park_offline_cpus ) free_percpu_area(cpu); break; + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(rc); diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 0b9631ac44..e76b129e6c 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -1661,6 +1661,9 @@ static int cf_check cpu_callback( case CPU_DEAD: psr_cpu_fini(cpu); break; + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(rc); diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 8aa621533f..5b9b196d58 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -1134,6 +1134,9 @@ static int cf_check cpu_smpboot_callback( case CPU_REMOVE: cpu_smpboot_free(cpu, true); break; + default: + /* Notifier pattern. */ + break; } return notifier_from_errno(rc); diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c index 212a99acd8..0fe4097544 100644 --- a/xen/common/rcupdate.c +++ b/xen/common/rcupdate.c @@ -657,6 +657,7 @@ static int cf_check cpu_callback( rcu_offline_cpu(&this_cpu(rcu_data), &rcu_ctrlblk, rdp); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index d84b65f197..dffa1ef476 100644 --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -2907,6 +2907,7 @@ static int cf_check cpu_schedule_callback( cpu_schedule_down(cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index ad8f608462..c7117f4243 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -1073,6 +1073,7 @@ static int cf_check cpu_callback( cpupool_cpu_remove_forced(cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 28c6e9d3ac..bf082478db 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -55,6 +55,7 @@ static int cf_check cpu_lockdebug_callback(struct notifier_block *nfb, break; default: + /* Notifier pattern. */ break; } diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c index 4c8d87a338..879b1f0d80 100644 --- a/xen/common/tasklet.c +++ b/xen/common/tasklet.c @@ -232,6 +232,7 @@ static int cf_check cpu_callback( migrate_tasklets_from_cpu(cpu, &per_cpu(softirq_tasklet_list, cpu)); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/common/timer.c b/xen/common/timer.c index a21798b76f..60e9a1493e 100644 --- a/xen/common/timer.c +++ b/xen/common/timer.c @@ -677,6 +677,7 @@ static int cf_check cpu_callback( break; default: + /* Notifier pattern. */ break; } diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index 8659ad3aee..9584b55398 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -682,6 +682,7 @@ static int cf_check cpu_callback( (void)cpufreq_del_cpu(cpu); break; default: + /* Notifier pattern. */ break; } diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c index d3627e4af7..e5b6be4794 100644 --- a/xen/drivers/passthrough/x86/hvm.c +++ b/xen/drivers/passthrough/x86/hvm.c @@ -1122,6 +1122,9 @@ static int cf_check cpu_callback( */ ASSERT(list_empty(&per_cpu(dpci_list, cpu))); break; + default: + /* Notifier pattern. */ + break; } return NOTIFY_DONE; diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index cc0062b027..f0c84eeb85 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -749,6 +749,9 @@ static int cf_check cpu_callback( if ( !page_list_empty(list) ) tasklet_schedule(tasklet); break; + default: + /* Notifier pattern. */ + break; } return NOTIFY_DONE;
MISRA C Rule 16.4 states that every `switch' statement shall have a `default' label" and a statement or a comment prior to the terminating break statement. This patch addresses some violations of the rule related to the "notifier pattern": a frequently-used pattern whereby only a few values are handled by the switch statement and nothing should be done for others (nothing to do in the default case). No functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/arch/arm/cpuerrata.c | 1 + xen/arch/arm/gic.c | 1 + xen/arch/arm/irq.c | 4 ++++ xen/arch/arm/mmu/p2m.c | 1 + xen/arch/arm/percpu.c | 1 + xen/arch/arm/smpboot.c | 1 + xen/arch/arm/time.c | 1 + xen/arch/arm/vgic-v3-its.c | 2 ++ xen/arch/x86/cpu/mcheck/mce.c | 4 ++++ xen/arch/x86/genapic/x2apic.c | 3 +++ xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/nmi.c | 1 + xen/arch/x86/percpu.c | 3 +++ xen/arch/x86/psr.c | 3 +++ xen/arch/x86/smpboot.c | 3 +++ xen/common/rcupdate.c | 1 + xen/common/sched/core.c | 1 + xen/common/sched/cpupool.c | 1 + xen/common/spinlock.c | 1 + xen/common/tasklet.c | 1 + xen/common/timer.c | 1 + xen/drivers/cpufreq/cpufreq.c | 1 + xen/drivers/passthrough/x86/hvm.c | 3 +++ xen/drivers/passthrough/x86/iommu.c | 3 +++ 24 files changed, 43 insertions(+)