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.1On 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
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.
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
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
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
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)
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
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.
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)
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
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.
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)
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
>
© 2016 - 2026 Red Hat, Inc.