[XEN PATCH 0/2] address some violations of MISRA C Rule 17.7

[XEN PATCH 0/2] address some violations of MISRA C Rule 17.7

[Nicola Vetrini]

MISRA C Rule 17.7 states: "The value returned by a function having non-void
return type shall be used". Therefore the functions that are subject to this
series are refactored to address these concerns.

Nicola Vetrini (2):
  xen/console: drop return value from consoled_guest_rx/tx
  xen/cpu: address MISRA C Rule 17.7

 xen/common/cpu.c            | 31 +++++++++++++++++++------------
 xen/drivers/char/consoled.c | 17 +++++------------
 xen/include/xen/consoled.h  |  4 ++--
 3 files changed, 26 insertions(+), 26 deletions(-)

-- 
2.34.1

[XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Nicola Vetrini]

These functions never saw a usage of their return value since
they were introduced, so it can be dropped since their usages
violate MISRA C Rule 17.7:
"The value returned by a function having non-void return type shall be used".

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/char/consoled.c | 17 +++++------------
 xen/include/xen/consoled.h  |  4 ++--
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
index 222e01844271..b415b632cecc 100644
--- a/xen/drivers/char/consoled.c
+++ b/xen/drivers/char/consoled.c
@@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
 static char buf[BUF_SZ + 1];
 
 /* Receives characters from a domain's PV console */
-size_t consoled_guest_rx(void)
+void consoled_guest_rx(void)
 {
-    size_t recv = 0, idx = 0;
+    size_t idx = 0;
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return 0;
+        return;
 
     spin_lock(&rx_lock);
 
@@ -73,7 +73,6 @@ size_t consoled_guest_rx(void)
         char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
 
         buf[idx++] = c;
-        recv++;
 
         if ( idx >= BUF_SZ )
         {
@@ -92,18 +91,15 @@ size_t consoled_guest_rx(void)
 
  out:
     spin_unlock(&rx_lock);
-
-    return recv;
 }
 
 /* Sends a character into a domain's PV console */
-size_t consoled_guest_tx(char c)
+void consoled_guest_tx(char c)
 {
-    size_t sent = 0;
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return 0;
+        return;
 
     cons = ACCESS_ONCE(cons_ring->in_cons);
     prod = cons_ring->in_prod;
@@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c)
         goto notify;
 
     cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
-    sent++;
 
     /* Write to the ring before updating the pointer */
     smp_wmb();
@@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c)
  notify:
     /* Always notify the guest: prevents receive path from getting stuck. */
     pv_shim_inject_evtchn(pv_console_evtchn());
-
-    return sent;
 }
 
 /*
diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
index 2b30516b3a0a..bd7ab6329ee8 100644
--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -5,8 +5,8 @@
 
 void consoled_set_ring_addr(struct xencons_interface *ring);
 struct xencons_interface *consoled_get_ring_addr(void);
-size_t consoled_guest_rx(void);
-size_t consoled_guest_tx(char c);
+void consoled_guest_rx(void);
+void consoled_guest_tx(char c);
 
 #endif /* __XEN_CONSOLED_H__ */
 /*
-- 
2.34.1

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Stefano Stabellini]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> These functions never saw a usage of their return value since
> they were introduced, so it can be dropped since their usages
> violate MISRA C Rule 17.7:
> "The value returned by a function having non-void return type shall be used".
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/drivers/char/consoled.c | 17 +++++------------
>  xen/include/xen/consoled.h  |  4 ++--
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
> index 222e01844271..b415b632cecc 100644
> --- a/xen/drivers/char/consoled.c
> +++ b/xen/drivers/char/consoled.c
> @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
>  static char buf[BUF_SZ + 1];
>  
>  /* Receives characters from a domain's PV console */
> -size_t consoled_guest_rx(void)
> +void consoled_guest_rx(void)
>  {
> -    size_t recv = 0, idx = 0;
> +    size_t idx = 0;
>      XENCONS_RING_IDX cons, prod;
>  
>      if ( !cons_ring )
> -        return 0;
> +        return;
>  
>      spin_lock(&rx_lock);
>  
> @@ -73,7 +73,6 @@ size_t consoled_guest_rx(void)
>          char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)];
>  
>          buf[idx++] = c;
> -        recv++;
>  
>          if ( idx >= BUF_SZ )
>          {
> @@ -92,18 +91,15 @@ size_t consoled_guest_rx(void)
>  
>   out:
>      spin_unlock(&rx_lock);
> -
> -    return recv;
>  }
>  
>  /* Sends a character into a domain's PV console */
> -size_t consoled_guest_tx(char c)
> +void consoled_guest_tx(char c)
>  {
> -    size_t sent = 0;
>      XENCONS_RING_IDX cons, prod;
>  
>      if ( !cons_ring )
> -        return 0;
> +        return;
>  
>      cons = ACCESS_ONCE(cons_ring->in_cons);
>      prod = cons_ring->in_prod;
> @@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c)
>          goto notify;
>  
>      cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
> -    sent++;
>  
>      /* Write to the ring before updating the pointer */
>      smp_wmb();
> @@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c)
>   notify:
>      /* Always notify the guest: prevents receive path from getting stuck. */
>      pv_shim_inject_evtchn(pv_console_evtchn());
> -
> -    return sent;
>  }
>  
>  /*
> diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
> index 2b30516b3a0a..bd7ab6329ee8 100644
> --- a/xen/include/xen/consoled.h
> +++ b/xen/include/xen/consoled.h
> @@ -5,8 +5,8 @@
>  
>  void consoled_set_ring_addr(struct xencons_interface *ring);
>  struct xencons_interface *consoled_get_ring_addr(void);
> -size_t consoled_guest_rx(void);
> -size_t consoled_guest_tx(char c);
> +void consoled_guest_rx(void);
> +void consoled_guest_tx(char c);
>  
>  #endif /* __XEN_CONSOLED_H__ */
>  /*
> -- 
> 2.34.1
> 

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Jan Beulich]

On 23.02.2024 23:56, Stefano Stabellini wrote:
> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
>> These functions never saw a usage of their return value since
>> they were introduced, so it can be dropped since their usages
>> violate MISRA C Rule 17.7:
>> "The value returned by a function having non-void return type shall be used".
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

The cleanup is certainly okay, but would one of you mind clarifying in how
far this code is relevant for certification? I don't expect there are plans
to run shim Xen in any projected production uses for which certification is
relevant? (The subject prefix is also unnecessarily wide here, when it's
only daemon code which is affected, not console code in general.)

Jan

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Nicola Vetrini]

On 2024-02-26 09:00, Jan Beulich wrote:
> On 23.02.2024 23:56, Stefano Stabellini wrote:
>> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
>>> These functions never saw a usage of their return value since
>>> they were introduced, so it can be dropped since their usages
>>> violate MISRA C Rule 17.7:
>>> "The value returned by a function having non-void return type shall 
>>> be used".
>>> 
>>> No functional change.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> 
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> The cleanup is certainly okay, but would one of you mind clarifying in 
> how
> far this code is relevant for certification? I don't expect there are 
> plans
> to run shim Xen in any projected production uses for which 
> certification is
> relevant? (The subject prefix is also unnecessarily wide here, when 
> it's
> only daemon code which is affected, not console code in general.)
> 

I agree on the subject prefix being too wide. The configuration that 
uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations 
that may never reach this condition this is relevant, unless its #ifdef 
is restricted to cases where the call may actually be reachable.
consoled_guest_rx was adjusted for uniformity, even though it's 
technically not included in the configuration under analysis at the 
moment.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Jan Beulich]

On 26.02.2024 09:23, Nicola Vetrini wrote:
> On 2024-02-26 09:00, Jan Beulich wrote:
>> On 23.02.2024 23:56, Stefano Stabellini wrote:
>>> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
>>>> These functions never saw a usage of their return value since
>>>> they were introduced, so it can be dropped since their usages
>>>> violate MISRA C Rule 17.7:
>>>> "The value returned by a function having non-void return type shall 
>>>> be used".
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> The cleanup is certainly okay, but would one of you mind clarifying in 
>> how
>> far this code is relevant for certification? I don't expect there are 
>> plans
>> to run shim Xen in any projected production uses for which 
>> certification is
>> relevant? (The subject prefix is also unnecessarily wide here, when 
>> it's
>> only daemon code which is affected, not console code in general.)
>>
> 
> I agree on the subject prefix being too wide. The configuration that 
> uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations 
> that may never reach this condition this is relevant, unless its #ifdef 
> is restricted to cases where the call may actually be reachable.

Hmm, I see. There are contradicting goals here then: It being just X86 is
to reduce the risk of someone overlooking a build breakage they may
introduce. Whereas for certification it's quite the other way around: We'd
like to "hide" as much code as possible.

Really I would have been inclined to suggest to drop the #ifdef, if
possible even without replacing by IS_ENABLED(), but instead leveraging
that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n.
After all that's a pattern we've been trying to follow. But with your
observation is becomes questionable whether extending use of IS_ENABLED()
is actually going to be helpful. Stefano - perhaps something to discuss
on one of the next meetings?

Jan

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Stefano Stabellini]

On Mon, 26 Feb 2024, Jan Beulich wrote:
> On 26.02.2024 09:23, Nicola Vetrini wrote:
> > On 2024-02-26 09:00, Jan Beulich wrote:
> >> On 23.02.2024 23:56, Stefano Stabellini wrote:
> >>> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> >>>> These functions never saw a usage of their return value since
> >>>> they were introduced, so it can be dropped since their usages
> >>>> violate MISRA C Rule 17.7:
> >>>> "The value returned by a function having non-void return type shall 
> >>>> be used".
> >>>>
> >>>> No functional change.
> >>>>
> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >> The cleanup is certainly okay, but would one of you mind clarifying in 
> >> how
> >> far this code is relevant for certification? I don't expect there are 
> >> plans
> >> to run shim Xen in any projected production uses for which 
> >> certification is
> >> relevant? (The subject prefix is also unnecessarily wide here, when 
> >> it's
> >> only daemon code which is affected, not console code in general.)
> >>
> > 
> > I agree on the subject prefix being too wide. The configuration that 
> > uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations 
> > that may never reach this condition this is relevant, unless its #ifdef 
> > is restricted to cases where the call may actually be reachable.
> 
> Hmm, I see. There are contradicting goals here then: It being just X86 is
> to reduce the risk of someone overlooking a build breakage they may
> introduce. Whereas for certification it's quite the other way around: We'd
> like to "hide" as much code as possible.
> 
> Really I would have been inclined to suggest to drop the #ifdef, if
> possible even without replacing by IS_ENABLED(), but instead leveraging
> that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n.

This is OK


> After all that's a pattern we've been trying to follow. But with your
> observation is becomes questionable whether extending use of IS_ENABLED()
> is actually going to be helpful. Stefano - perhaps something to discuss
> on one of the next meetings?

Yes. I checked with the safety manager and his opinion is that
IS_ENABLED() is OK to use as a way to disable code from a safety
perspective.

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Jan Beulich]

On 26.02.2024 23:49, Stefano Stabellini wrote:
> On Mon, 26 Feb 2024, Jan Beulich wrote:
>> On 26.02.2024 09:23, Nicola Vetrini wrote:
>>> On 2024-02-26 09:00, Jan Beulich wrote:
>>>> On 23.02.2024 23:56, Stefano Stabellini wrote:
>>>>> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
>>>>>> These functions never saw a usage of their return value since
>>>>>> they were introduced, so it can be dropped since their usages
>>>>>> violate MISRA C Rule 17.7:
>>>>>> "The value returned by a function having non-void return type shall 
>>>>>> be used".
>>>>>>
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>
>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>> The cleanup is certainly okay, but would one of you mind clarifying in 
>>>> how
>>>> far this code is relevant for certification? I don't expect there are 
>>>> plans
>>>> to run shim Xen in any projected production uses for which 
>>>> certification is
>>>> relevant? (The subject prefix is also unnecessarily wide here, when 
>>>> it's
>>>> only daemon code which is affected, not console code in general.)
>>>>
>>>
>>> I agree on the subject prefix being too wide. The configuration that 
>>> uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations 
>>> that may never reach this condition this is relevant, unless its #ifdef 
>>> is restricted to cases where the call may actually be reachable.
>>
>> Hmm, I see. There are contradicting goals here then: It being just X86 is
>> to reduce the risk of someone overlooking a build breakage they may
>> introduce. Whereas for certification it's quite the other way around: We'd
>> like to "hide" as much code as possible.
>>
>> Really I would have been inclined to suggest to drop the #ifdef, if
>> possible even without replacing by IS_ENABLED(), but instead leveraging
>> that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n.
> 
> This is OK
> 
> 
>> After all that's a pattern we've been trying to follow. But with your
>> observation is becomes questionable whether extending use of IS_ENABLED()
>> is actually going to be helpful. Stefano - perhaps something to discuss
>> on one of the next meetings?
> 
> Yes. I checked with the safety manager and his opinion is that
> IS_ENABLED() is OK to use as a way to disable code from a safety
> perspective.

Yet unlike when #ifdef is used, such code would remain visible to e.g.
Eclair even after the preprocessing step. Note the context in which
I'm bringing this up - if IS_ENABLED() was properly used here (and as
tightly as possible), the tool would still have complained, aiui.

Jan

Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Stefano Stabellini]

On Tue, 27 Feb 2024, Jan Beulich wrote:
> On 26.02.2024 23:49, Stefano Stabellini wrote:
> > On Mon, 26 Feb 2024, Jan Beulich wrote:
> >> On 26.02.2024 09:23, Nicola Vetrini wrote:
> >>> On 2024-02-26 09:00, Jan Beulich wrote:
> >>>> On 23.02.2024 23:56, Stefano Stabellini wrote:
> >>>>> On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> >>>>>> These functions never saw a usage of their return value since
> >>>>>> they were introduced, so it can be dropped since their usages
> >>>>>> violate MISRA C Rule 17.7:
> >>>>>> "The value returned by a function having non-void return type shall 
> >>>>>> be used".
> >>>>>>
> >>>>>> No functional change.
> >>>>>>
> >>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>>>
> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>
> >>>> The cleanup is certainly okay, but would one of you mind clarifying in 
> >>>> how
> >>>> far this code is relevant for certification? I don't expect there are 
> >>>> plans
> >>>> to run shim Xen in any projected production uses for which 
> >>>> certification is
> >>>> relevant? (The subject prefix is also unnecessarily wide here, when 
> >>>> it's
> >>>> only daemon code which is affected, not console code in general.)
> >>>>
> >>>
> >>> I agree on the subject prefix being too wide. The configuration that 
> >>> uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations 
> >>> that may never reach this condition this is relevant, unless its #ifdef 
> >>> is restricted to cases where the call may actually be reachable.
> >>
> >> Hmm, I see. There are contradicting goals here then: It being just X86 is
> >> to reduce the risk of someone overlooking a build breakage they may
> >> introduce. Whereas for certification it's quite the other way around: We'd
> >> like to "hide" as much code as possible.
> >>
> >> Really I would have been inclined to suggest to drop the #ifdef, if
> >> possible even without replacing by IS_ENABLED(), but instead leveraging
> >> that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n.
> > 
> > This is OK
> > 
> > 
> >> After all that's a pattern we've been trying to follow. But with your
> >> observation is becomes questionable whether extending use of IS_ENABLED()
> >> is actually going to be helpful. Stefano - perhaps something to discuss
> >> on one of the next meetings?
> > 
> > Yes. I checked with the safety manager and his opinion is that
> > IS_ENABLED() is OK to use as a way to disable code from a safety
> > perspective.
> 
> Yet unlike when #ifdef is used, such code would remain visible to e.g.
> Eclair even after the preprocessing step. Note the context in which
> I'm bringing this up - if IS_ENABLED() was properly used here (and as
> tightly as possible), the tool would still have complained, aiui.

Let me check with Roberto about this.

[XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Nicola Vetrini]

Refactor cpu_notifier_call_chain into two functions:
- the variant that is allowed to fail loses the nofail flag
- the variant that shouldn't fail is encapsulated in a call
  to the failing variant, with an additional check.

This prevents uses of the function that are not supposed to
fail from ignoring the return value, thus violating Rule 17.7:
"The value returned by a function having non-void return type shall
be used".

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/cpu.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 8709db4d2957..0b7cf54c4264 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -78,20 +78,27 @@ void __init register_cpu_notifier(struct notifier_block *nb)
 }
 
 static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
-                                   struct notifier_block **nb, bool nofail)
+                                   struct notifier_block **nb)
 {
     void *hcpu = (void *)(long)cpu;
     int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
     int ret =  notifier_to_errno(notifier_rc);
 
-    BUG_ON(ret && nofail);
-
     return ret;
 }
 
+static void cpu_notifier_call_chain_nofail(unsigned int cpu,
+                                           unsigned long action,
+                                           struct notifier_block **nb)
+{
+    int ret = cpu_notifier_call_chain(cpu, action, nb);
+
+    BUG_ON(ret);
+}
+
 static void cf_check _take_cpu_down(void *unused)
 {
-    cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL, true);
+    cpu_notifier_call_chain_nofail(smp_processor_id(), CPU_DYING, NULL);
     __cpu_disable();
 }
 
@@ -116,7 +123,7 @@ int cpu_down(unsigned int cpu)
     if ( !cpu_online(cpu) )
         goto out;
 
-    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb, false);
+    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb);
     if ( err )
         goto fail;
 
@@ -129,14 +136,14 @@ int cpu_down(unsigned int cpu)
     err = cpu_online(cpu);
     BUG_ON(err);
 
-    cpu_notifier_call_chain(cpu, CPU_DEAD, NULL, true);
+    cpu_notifier_call_chain_nofail(cpu, CPU_DEAD, NULL);
 
     send_global_virq(VIRQ_PCPU_STATE);
     cpu_hotplug_done();
     return 0;
 
  fail:
-    cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, &nb, true);
+    cpu_notifier_call_chain_nofail(cpu, CPU_DOWN_FAILED, &nb);
  out:
     cpu_hotplug_done();
     return err;
@@ -157,7 +164,7 @@ int cpu_up(unsigned int cpu)
     if ( cpu_online(cpu) )
         goto out;
 
-    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb, false);
+    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb);
     if ( err )
         goto fail;
 
@@ -165,7 +172,7 @@ int cpu_up(unsigned int cpu)
     if ( err < 0 )
         goto fail;
 
-    cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL, true);
+    cpu_notifier_call_chain_nofail(cpu, CPU_ONLINE, NULL);
 
     send_global_virq(VIRQ_PCPU_STATE);
 
@@ -173,7 +180,7 @@ int cpu_up(unsigned int cpu)
     return 0;
 
  fail:
-    cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, &nb, true);
+    cpu_notifier_call_chain_nofail(cpu, CPU_UP_CANCELED, &nb);
  out:
     cpu_hotplug_done();
     return err;
@@ -181,7 +188,7 @@ int cpu_up(unsigned int cpu)
 
 void notify_cpu_starting(unsigned int cpu)
 {
-    cpu_notifier_call_chain(cpu, CPU_STARTING, NULL, true);
+    cpu_notifier_call_chain_nofail(cpu, CPU_STARTING, NULL);
 }
 
 static cpumask_t frozen_cpus;
@@ -237,7 +244,7 @@ void enable_nonboot_cpus(void)
     }
 
     for_each_cpu ( cpu, &frozen_cpus )
-        cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL, true);
+        cpu_notifier_call_chain_nofail(cpu, CPU_RESUME_FAILED, NULL);
 
     cpumask_clear(&frozen_cpus);
 }
-- 
2.34.1

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Stefano Stabellini]

On Fri, 23 Feb 2024, Nicola Vetrini wrote:
> Refactor cpu_notifier_call_chain into two functions:
> - the variant that is allowed to fail loses the nofail flag
> - the variant that shouldn't fail is encapsulated in a call
>   to the failing variant, with an additional check.
> 
> This prevents uses of the function that are not supposed to
> fail from ignoring the return value, thus violating Rule 17.7:
> "The value returned by a function having non-void return type shall
> be used".
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/common/cpu.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 8709db4d2957..0b7cf54c4264 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -78,20 +78,27 @@ void __init register_cpu_notifier(struct notifier_block *nb)
>  }
>  
>  static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
> -                                   struct notifier_block **nb, bool nofail)
> +                                   struct notifier_block **nb)
>  {
>      void *hcpu = (void *)(long)cpu;
>      int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
>      int ret =  notifier_to_errno(notifier_rc);
>  
> -    BUG_ON(ret && nofail);
> -
>      return ret;
>  }
>  
> +static void cpu_notifier_call_chain_nofail(unsigned int cpu,
> +                                           unsigned long action,
> +                                           struct notifier_block **nb)
> +{
> +    int ret = cpu_notifier_call_chain(cpu, action, nb);
> +
> +    BUG_ON(ret);
> +}
> +
>  static void cf_check _take_cpu_down(void *unused)
>  {
> -    cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL, true);
> +    cpu_notifier_call_chain_nofail(smp_processor_id(), CPU_DYING, NULL);
>      __cpu_disable();
>  }
>  
> @@ -116,7 +123,7 @@ int cpu_down(unsigned int cpu)
>      if ( !cpu_online(cpu) )
>          goto out;
>  
> -    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb, false);
> +    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb);
>      if ( err )
>          goto fail;
>  
> @@ -129,14 +136,14 @@ int cpu_down(unsigned int cpu)
>      err = cpu_online(cpu);
>      BUG_ON(err);
>  
> -    cpu_notifier_call_chain(cpu, CPU_DEAD, NULL, true);
> +    cpu_notifier_call_chain_nofail(cpu, CPU_DEAD, NULL);
>  
>      send_global_virq(VIRQ_PCPU_STATE);
>      cpu_hotplug_done();
>      return 0;
>  
>   fail:
> -    cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, &nb, true);
> +    cpu_notifier_call_chain_nofail(cpu, CPU_DOWN_FAILED, &nb);
>   out:
>      cpu_hotplug_done();
>      return err;
> @@ -157,7 +164,7 @@ int cpu_up(unsigned int cpu)
>      if ( cpu_online(cpu) )
>          goto out;
>  
> -    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb, false);
> +    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb);
>      if ( err )
>          goto fail;
>  
> @@ -165,7 +172,7 @@ int cpu_up(unsigned int cpu)
>      if ( err < 0 )
>          goto fail;
>  
> -    cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL, true);
> +    cpu_notifier_call_chain_nofail(cpu, CPU_ONLINE, NULL);
>  
>      send_global_virq(VIRQ_PCPU_STATE);
>  
> @@ -173,7 +180,7 @@ int cpu_up(unsigned int cpu)
>      return 0;
>  
>   fail:
> -    cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, &nb, true);
> +    cpu_notifier_call_chain_nofail(cpu, CPU_UP_CANCELED, &nb);
>   out:
>      cpu_hotplug_done();
>      return err;
> @@ -181,7 +188,7 @@ int cpu_up(unsigned int cpu)
>  
>  void notify_cpu_starting(unsigned int cpu)
>  {
> -    cpu_notifier_call_chain(cpu, CPU_STARTING, NULL, true);
> +    cpu_notifier_call_chain_nofail(cpu, CPU_STARTING, NULL);
>  }
>  
>  static cpumask_t frozen_cpus;
> @@ -237,7 +244,7 @@ void enable_nonboot_cpus(void)
>      }
>  
>      for_each_cpu ( cpu, &frozen_cpus )
> -        cpu_notifier_call_chain(cpu, CPU_RESUME_FAILED, NULL, true);
> +        cpu_notifier_call_chain_nofail(cpu, CPU_RESUME_FAILED, NULL);
>  
>      cpumask_clear(&frozen_cpus);
>  }
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Jan Beulich]

On 23.02.2024 10:35, Nicola Vetrini wrote:
> Refactor cpu_notifier_call_chain into two functions:
> - the variant that is allowed to fail loses the nofail flag
> - the variant that shouldn't fail is encapsulated in a call
>   to the failing variant, with an additional check.
> 
> This prevents uses of the function that are not supposed to
> fail from ignoring the return value, thus violating Rule 17.7:
> "The value returned by a function having non-void return type shall
> be used".
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

I'm afraid I disagree with this kind of bifurcation. No matter what
Misra thinks or says, it is normal for return values of functions to
not always be relevant to check. To deal with the Misra rule imo
requires to first have an abstract plan of how to handle such
globally in the code base. Imo such a plan can't be to introduce
perhaps dozens of new wrapper functions like is done here.

Jan

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Stefano Stabellini]

On Mon, 26 Feb 2024, Jan Beulich wrote:
> On 23.02.2024 10:35, Nicola Vetrini wrote:
> > Refactor cpu_notifier_call_chain into two functions:
> > - the variant that is allowed to fail loses the nofail flag
> > - the variant that shouldn't fail is encapsulated in a call
> >   to the failing variant, with an additional check.
> > 
> > This prevents uses of the function that are not supposed to
> > fail from ignoring the return value, thus violating Rule 17.7:
> > "The value returned by a function having non-void return type shall
> > be used".
> > 
> > No functional change.
> > 
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> I'm afraid I disagree with this kind of bifurcation. No matter what
> Misra thinks or says, it is normal for return values of functions to
> not always be relevant to check.

Hi Jan, I disagree.

Regardless of MISRA, I really think return values need to be checked.
Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
return values to be checked. This patch is a good step forward.


> To deal with the Misra rule imo requires to first have an abstract
> plan of how to handle such globally in the code base. Imo such a plan
> can't be to introduce perhaps dozens of new wrapper functions like is
> done here.

This patch is following the right pattern, one we already follow with
the _locked suffix.

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Jan Beulich]

On 27.02.2024 01:26, Stefano Stabellini wrote:
> On Mon, 26 Feb 2024, Jan Beulich wrote:
>> On 23.02.2024 10:35, Nicola Vetrini wrote:
>>> Refactor cpu_notifier_call_chain into two functions:
>>> - the variant that is allowed to fail loses the nofail flag
>>> - the variant that shouldn't fail is encapsulated in a call
>>>   to the failing variant, with an additional check.
>>>
>>> This prevents uses of the function that are not supposed to
>>> fail from ignoring the return value, thus violating Rule 17.7:
>>> "The value returned by a function having non-void return type shall
>>> be used".
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> I'm afraid I disagree with this kind of bifurcation. No matter what
>> Misra thinks or says, it is normal for return values of functions to
>> not always be relevant to check.
> 
> Hi Jan, I disagree.
> 
> Regardless of MISRA, I really think return values need to be checked.
> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
> return values to be checked. This patch is a good step forward.

Yet splitting functions isn't the only way to deal with Misra's
requirements, I suppose. After all there are functions where the
return value is purely courtesy for perhaps just one of its callers.

Splitting simply doesn't scale very well, imo.

>> To deal with the Misra rule imo requires to first have an abstract
>> plan of how to handle such globally in the code base. Imo such a plan
>> can't be to introduce perhaps dozens of new wrapper functions like is
>> done here.
> 
> This patch is following the right pattern, one we already follow with
> the _locked suffix.

Right, and - just to mention it - one which I similarly dislike, albeit
to a lesser degree.

Jan

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Julien Grall]

Hi Jan,

On 27/02/2024 07:28, Jan Beulich wrote:
> On 27.02.2024 01:26, Stefano Stabellini wrote:
>> On Mon, 26 Feb 2024, Jan Beulich wrote:
>>> On 23.02.2024 10:35, Nicola Vetrini wrote:
>>>> Refactor cpu_notifier_call_chain into two functions:
>>>> - the variant that is allowed to fail loses the nofail flag
>>>> - the variant that shouldn't fail is encapsulated in a call
>>>>    to the failing variant, with an additional check.
>>>>
>>>> This prevents uses of the function that are not supposed to
>>>> fail from ignoring the return value, thus violating Rule 17.7:
>>>> "The value returned by a function having non-void return type shall
>>>> be used".
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>
>>> I'm afraid I disagree with this kind of bifurcation. No matter what
>>> Misra thinks or says, it is normal for return values of functions to
>>> not always be relevant to check.
>>
>> Hi Jan, I disagree.
>>
>> Regardless of MISRA, I really think return values need to be checked.
>> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
>> return values to be checked. This patch is a good step forward.
> 
> Yet splitting functions isn't the only way to deal with Misra's
> requirements, I suppose. After all there are functions where the
> return value is purely courtesy for perhaps just one of its callers.

You are right that we have some places where one caller care about the 
return value. But the problem is how do you tell whether the return was 
ignored on purpose or not?

We had at least one XSA because the return value of a function was not 
checked (see XSA-222). We also had plenty of smaller patches to check 
returns.

So far, we added __must_check when we believed return values should be 
checked. But usually at the point we notice, this is far too late.

To me the goal should be that we enforce __must_check everywhere. We are 
probably going to detect places where we forgot to check the return. For 
thoses that are on purpose, we can document them.

> 
> Splitting simply doesn't scale very well, imo.

Do you have another proposal? As Stefano said, we adopted the rule 17.7. 
So we know need a solution to address it.

> 
>>> To deal with the Misra rule imo requires to first have an abstract
>>> plan of how to handle such globally in the code base. Imo such a plan
>>> can't be to introduce perhaps dozens of new wrapper functions like is
>>> done here.
>>
>> This patch is following the right pattern, one we already follow with
>> the _locked suffix.
> 
> Right, and - just to mention it - one which I similarly dislike, albeit
> to a lesser degree.

AFAIU, we are debating between having a boolean indicating if a function 
doesn't fail or adding a wrapper.

While I understand this requires to add more code, the advantage of the 
suffix is this is more obvious for the reader (including the reviewer) 
that the call is not supposed to fail. I agree this is a matter of taste 
here...

In this case, what would be your suggestion to address the problem?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Jan Beulich]

On 27.02.2024 12:52, Julien Grall wrote:
> Hi Jan,
> 
> On 27/02/2024 07:28, Jan Beulich wrote:
>> On 27.02.2024 01:26, Stefano Stabellini wrote:
>>> On Mon, 26 Feb 2024, Jan Beulich wrote:
>>>> On 23.02.2024 10:35, Nicola Vetrini wrote:
>>>>> Refactor cpu_notifier_call_chain into two functions:
>>>>> - the variant that is allowed to fail loses the nofail flag
>>>>> - the variant that shouldn't fail is encapsulated in a call
>>>>>    to the failing variant, with an additional check.
>>>>>
>>>>> This prevents uses of the function that are not supposed to
>>>>> fail from ignoring the return value, thus violating Rule 17.7:
>>>>> "The value returned by a function having non-void return type shall
>>>>> be used".
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>
>>>> I'm afraid I disagree with this kind of bifurcation. No matter what
>>>> Misra thinks or says, it is normal for return values of functions to
>>>> not always be relevant to check.
>>>
>>> Hi Jan, I disagree.
>>>
>>> Regardless of MISRA, I really think return values need to be checked.
>>> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
>>> return values to be checked. This patch is a good step forward.
>>
>> Yet splitting functions isn't the only way to deal with Misra's
>> requirements, I suppose. After all there are functions where the
>> return value is purely courtesy for perhaps just one of its callers.
> 
> You are right that we have some places where one caller care about the 
> return value. But the problem is how do you tell whether the return was 
> ignored on purpose or not?
> 
> We had at least one XSA because the return value of a function was not 
> checked (see XSA-222). We also had plenty of smaller patches to check 
> returns.
> 
> So far, we added __must_check when we believed return values should be 
> checked. But usually at the point we notice, this is far too late.
> 
> To me the goal should be that we enforce __must_check everywhere. We are 
> probably going to detect places where we forgot to check the return. For 
> thoses that are on purpose, we can document them.
> 
>>
>> Splitting simply doesn't scale very well, imo.
> 
> Do you have another proposal? As Stefano said, we adopted the rule 17.7. 
> So we know need a solution to address it.

One possibility that was circulated while discussing was to add (void)
casts. I'm not a huge fan of those, but between the two options that
might be the lesser evil. We also use funny (should I say ugly)
workarounds in a few cases where we have __must_check but still want
to not really handle the return value in certain cases. Given there are
example in the code base, extending use of such constructs is certainly
also something that may want considering.

Jan

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Stefano Stabellini]

On Tue, 27 Feb 2024, Jan Beulich wrote:
> On 27.02.2024 12:52, Julien Grall wrote:
> > Hi Jan,
> > 
> > On 27/02/2024 07:28, Jan Beulich wrote:
> >> On 27.02.2024 01:26, Stefano Stabellini wrote:
> >>> On Mon, 26 Feb 2024, Jan Beulich wrote:
> >>>> On 23.02.2024 10:35, Nicola Vetrini wrote:
> >>>>> Refactor cpu_notifier_call_chain into two functions:
> >>>>> - the variant that is allowed to fail loses the nofail flag
> >>>>> - the variant that shouldn't fail is encapsulated in a call
> >>>>>    to the failing variant, with an additional check.
> >>>>>
> >>>>> This prevents uses of the function that are not supposed to
> >>>>> fail from ignoring the return value, thus violating Rule 17.7:
> >>>>> "The value returned by a function having non-void return type shall
> >>>>> be used".
> >>>>>
> >>>>> No functional change.
> >>>>>
> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>>
> >>>> I'm afraid I disagree with this kind of bifurcation. No matter what
> >>>> Misra thinks or says, it is normal for return values of functions to
> >>>> not always be relevant to check.
> >>>
> >>> Hi Jan, I disagree.
> >>>
> >>> Regardless of MISRA, I really think return values need to be checked.
> >>> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
> >>> return values to be checked. This patch is a good step forward.
> >>
> >> Yet splitting functions isn't the only way to deal with Misra's
> >> requirements, I suppose. After all there are functions where the
> >> return value is purely courtesy for perhaps just one of its callers.
> > 
> > You are right that we have some places where one caller care about the 
> > return value. But the problem is how do you tell whether the return was 
> > ignored on purpose or not?
> > 
> > We had at least one XSA because the return value of a function was not 
> > checked (see XSA-222). We also had plenty of smaller patches to check 
> > returns.
> > 
> > So far, we added __must_check when we believed return values should be 
> > checked. But usually at the point we notice, this is far too late.
> > 
> > To me the goal should be that we enforce __must_check everywhere. We are 
> > probably going to detect places where we forgot to check the return. For 
> > thoses that are on purpose, we can document them.
> > 
> >>
> >> Splitting simply doesn't scale very well, imo.
> > 
> > Do you have another proposal? As Stefano said, we adopted the rule 17.7. 
> > So we know need a solution to address it.
> 
> One possibility that was circulated while discussing was to add (void)
> casts. I'm not a huge fan of those, but between the two options that
> might be the lesser evil. We also use funny (should I say ugly)
> workarounds in a few cases where we have __must_check but still want
> to not really handle the return value in certain cases. Given there are
> example in the code base, extending use of such constructs is certainly
> also something that may want considering.

I asked Roberto if void casts are an option for compliance.

In any case, I don't think we should use void casts in the specific
cases this patch is dealing with. Void casts (if anything) should be a
last resort while this patch fixes the issue in a better way.

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Nicola Vetrini]

On 2024-02-28 03:10, Stefano Stabellini wrote:
> On Tue, 27 Feb 2024, Jan Beulich wrote:
>> On 27.02.2024 12:52, Julien Grall wrote:
>> > Hi Jan,
>> >
>> > On 27/02/2024 07:28, Jan Beulich wrote:
>> >> On 27.02.2024 01:26, Stefano Stabellini wrote:
>> >>> On Mon, 26 Feb 2024, Jan Beulich wrote:
>> >>>> On 23.02.2024 10:35, Nicola Vetrini wrote:
>> >>>>> Refactor cpu_notifier_call_chain into two functions:
>> >>>>> - the variant that is allowed to fail loses the nofail flag
>> >>>>> - the variant that shouldn't fail is encapsulated in a call
>> >>>>>    to the failing variant, with an additional check.
>> >>>>>
>> >>>>> This prevents uses of the function that are not supposed to
>> >>>>> fail from ignoring the return value, thus violating Rule 17.7:
>> >>>>> "The value returned by a function having non-void return type shall
>> >>>>> be used".
>> >>>>>
>> >>>>> No functional change.
>> >>>>>
>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> >>>>
>> >>>> I'm afraid I disagree with this kind of bifurcation. No matter what
>> >>>> Misra thinks or says, it is normal for return values of functions to
>> >>>> not always be relevant to check.
>> >>>
>> >>> Hi Jan, I disagree.
>> >>>
>> >>> Regardless of MISRA, I really think return values need to be checked.
>> >>> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
>> >>> return values to be checked. This patch is a good step forward.
>> >>
>> >> Yet splitting functions isn't the only way to deal with Misra's
>> >> requirements, I suppose. After all there are functions where the
>> >> return value is purely courtesy for perhaps just one of its callers.
>> >
>> > You are right that we have some places where one caller care about the
>> > return value. But the problem is how do you tell whether the return was
>> > ignored on purpose or not?
>> >
>> > We had at least one XSA because the return value of a function was not
>> > checked (see XSA-222). We also had plenty of smaller patches to check
>> > returns.
>> >
>> > So far, we added __must_check when we believed return values should be
>> > checked. But usually at the point we notice, this is far too late.
>> >
>> > To me the goal should be that we enforce __must_check everywhere. We are
>> > probably going to detect places where we forgot to check the return. For
>> > thoses that are on purpose, we can document them.
>> >
>> >>
>> >> Splitting simply doesn't scale very well, imo.
>> >
>> > Do you have another proposal? As Stefano said, we adopted the rule 17.7.
>> > So we know need a solution to address it.
>> 
>> One possibility that was circulated while discussing was to add (void)
>> casts. I'm not a huge fan of those, but between the two options that
>> might be the lesser evil. We also use funny (should I say ugly)
>> workarounds in a few cases where we have __must_check but still want
>> to not really handle the return value in certain cases. Given there 
>> are
>> example in the code base, extending use of such constructs is 
>> certainly
>> also something that may want considering.
> 
> I asked Roberto if void casts are an option for compliance.
> 

void casts are an option for sure. The rationale for the rule explicitly 
lists them as a compliance mechanism. An interesting aspect is what 
would be the consensus around void casts on functions whose return value 
is always ignored vs. functions whose return value is sometimes ignored.

> In any case, I don't think we should use void casts in the specific
> cases this patch is dealing with. Void casts (if anything) should be a
> last resort while this patch fixes the issue in a better way.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Julien Grall]

Hi Nicola,

On 28/02/2024 11:09, Nicola Vetrini wrote:
>> I asked Roberto if void casts are an option for compliance.
>>
> 
> void casts are an option for sure. The rationale for the rule explicitly 
> lists them as a compliance mechanism. An interesting aspect is what 
> would be the consensus around void casts on functions whose return value 
> is always ignored vs. functions whose return value is sometimes ignored.

If a return is always ignored, then the function should return void. For 
the second case, I think it will be on the case by case basis.

> 
>> In any case, I don't think we should use void casts in the specific
>> cases this patch is dealing with. Void casts (if anything) should be a
>> last resort while this patch fixes the issue in a better way.

+1.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Stefano Stabellini]

On Wed, 28 Feb 2024, Julien Grall wrote:
> Hi Nicola,
> 
> On 28/02/2024 11:09, Nicola Vetrini wrote:
> > > I asked Roberto if void casts are an option for compliance.
> > > 
> > 
> > void casts are an option for sure. The rationale for the rule explicitly
> > lists them as a compliance mechanism. An interesting aspect is what would be
> > the consensus around void casts on functions whose return value is always
> > ignored vs. functions whose return value is sometimes ignored.
> 
> If a return is always ignored, then the function should return void. For the
> second case, I think it will be on the case by case basis.

+1

 
> > > In any case, I don't think we should use void casts in the specific
> > > cases this patch is dealing with. Void casts (if anything) should be a
> > > last resort while this patch fixes the issue in a better way.
> 
> +1.

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Nicola Vetrini]

On 2024-02-28 23:38, Stefano Stabellini wrote:
> On Wed, 28 Feb 2024, Julien Grall wrote:
>> Hi Nicola,
>> 
>> On 28/02/2024 11:09, Nicola Vetrini wrote:
>> > > I asked Roberto if void casts are an option for compliance.
>> > >
>> >
>> > void casts are an option for sure. The rationale for the rule explicitly
>> > lists them as a compliance mechanism. An interesting aspect is what would be
>> > the consensus around void casts on functions whose return value is always
>> > ignored vs. functions whose return value is sometimes ignored.
>> 
>> If a return is always ignored, then the function should return void. 
>> For the
>> second case, I think it will be on the case by case basis.
> 
> +1
> 

Ok, noted.

> 
>> > > In any case, I don't think we should use void casts in the specific
>> > > cases this patch is dealing with. Void casts (if anything) should be a
>> > > last resort while this patch fixes the issue in a better way.
>> 
>> +1.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Nicola Vetrini]

On 2024-02-27 13:47, Jan Beulich wrote:
> On 27.02.2024 12:52, Julien Grall wrote:
>> Hi Jan,
>> 
>> On 27/02/2024 07:28, Jan Beulich wrote:
>>> On 27.02.2024 01:26, Stefano Stabellini wrote:
>>>> On Mon, 26 Feb 2024, Jan Beulich wrote:
>>>>> On 23.02.2024 10:35, Nicola Vetrini wrote:
>>>>>> Refactor cpu_notifier_call_chain into two functions:
>>>>>> - the variant that is allowed to fail loses the nofail flag
>>>>>> - the variant that shouldn't fail is encapsulated in a call
>>>>>>    to the failing variant, with an additional check.
>>>>>> 
>>>>>> This prevents uses of the function that are not supposed to
>>>>>> fail from ignoring the return value, thus violating Rule 17.7:
>>>>>> "The value returned by a function having non-void return type 
>>>>>> shall
>>>>>> be used".
>>>>>> 
>>>>>> No functional change.
>>>>>> 
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> 
>>>>> I'm afraid I disagree with this kind of bifurcation. No matter what
>>>>> Misra thinks or says, it is normal for return values of functions 
>>>>> to
>>>>> not always be relevant to check.
>>>> 
>>>> Hi Jan, I disagree.
>>>> 
>>>> Regardless of MISRA, I really think return values need to be 
>>>> checked.
>>>> Moreover, we decided as a group to honor MISRA Rule 17.7, which 
>>>> requires
>>>> return values to be checked. This patch is a good step forward.
>>> 
>>> Yet splitting functions isn't the only way to deal with Misra's
>>> requirements, I suppose. After all there are functions where the
>>> return value is purely courtesy for perhaps just one of its callers.
>> 
>> You are right that we have some places where one caller care about the
>> return value. But the problem is how do you tell whether the return 
>> was
>> ignored on purpose or not?
>> 
>> We had at least one XSA because the return value of a function was not
>> checked (see XSA-222). We also had plenty of smaller patches to check
>> returns.
>> 
>> So far, we added __must_check when we believed return values should be
>> checked. But usually at the point we notice, this is far too late.
>> 
>> To me the goal should be that we enforce __must_check everywhere. We 
>> are
>> probably going to detect places where we forgot to check the return. 
>> For
>> thoses that are on purpose, we can document them.
>> 
>>> 
>>> Splitting simply doesn't scale very well, imo.
>> 

Yes. I certainly don't plan to do much splitting either. I saw this one 
as a low-hanging fruit.

>> Do you have another proposal? As Stefano said, we adopted the rule 
>> 17.7.
>> So we know need a solution to address it.
> 
> One possibility that was circulated while discussing was to add (void)
> casts. I'm not a huge fan of those, but between the two options that
> might be the lesser evil. We also use funny (should I say ugly)
> workarounds in a few cases where we have __must_check but still want
> to not really handle the return value in certain cases. Given there are
> example in the code base, extending use of such constructs is certainly
> also something that may want considering.
> 

Can you point out some of these constructs, just to get an idea of what 
that might look like?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Jan Beulich]

On 28.02.2024 12:06, Nicola Vetrini wrote:
> On 2024-02-27 13:47, Jan Beulich wrote:
>> On 27.02.2024 12:52, Julien Grall wrote:
>>> Do you have another proposal? As Stefano said, we adopted the rule 
>>> 17.7.
>>> So we know need a solution to address it.
>>
>> One possibility that was circulated while discussing was to add (void)
>> casts. I'm not a huge fan of those, but between the two options that
>> might be the lesser evil. We also use funny (should I say ugly)
>> workarounds in a few cases where we have __must_check but still want
>> to not really handle the return value in certain cases. Given there are
>> example in the code base, extending use of such constructs is certainly
>> also something that may want considering.
> 
> Can you point out some of these constructs, just to get an idea of what 
> that might look like?

Grep for __must_check used in comments.

Jan