common/sched: address a violation of MISRA C Rule 17.7

[XEN PATCH] common/sched: address a violation of MISRA C Rule 17.7

Posted by victorm.lira@amd.com 2 months, 2 weeks ago

From: Victor Lira <victorm.lira@amd.com>

Rule 17.7: "The value returned by a function having non-void return type
shall be used"

This patch fixes this by adding a check to the return value.
No functional changes.

Signed-off-by: Victor Lira <victorm.lira@amd.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Dario Faggioli <dfaggioli@suse.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
---
 xen/common/sched/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d84b65f197..e1cd824622 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2789,7 +2789,10 @@ static int cpu_schedule_up(unsigned int cpu)
     BUG_ON(cpu >= NR_CPUS);
 
     if ( idle_vcpu[cpu] == NULL )
-        vcpu_create(idle_vcpu[0]->domain, cpu);
+    {
+        if ( vcpu_create(idle_vcpu[0]->domain, cpu) == NULL )
+            return -ENOMEM;
+    }
     else
         idle_vcpu[cpu]->sched_unit->res = sr;
 
-- 
2.37.6

Re: [XEN PATCH] common/sched: address a violation of MISRA C Rule 17.7

Posted by Jan Beulich 2 months, 2 weeks ago

On 25.06.2024 02:15, victorm.lira@amd.com wrote:
> From: Victor Lira <victorm.lira@amd.com>
> 
> Rule 17.7: "The value returned by a function having non-void return type
> shall be used"
> 
> This patch fixes this by adding a check to the return value.
> No functional changes.
> 
> Signed-off-by: Victor Lira <victorm.lira@amd.com>
> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Dario Faggioli <dfaggioli@suse.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: xen-devel@lists.xenproject.org
> ---
>  xen/common/sched/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index d84b65f197..e1cd824622 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2789,7 +2789,10 @@ static int cpu_schedule_up(unsigned int cpu)
>      BUG_ON(cpu >= NR_CPUS);
>  
>      if ( idle_vcpu[cpu] == NULL )
> -        vcpu_create(idle_vcpu[0]->domain, cpu);
> +    {
> +        if ( vcpu_create(idle_vcpu[0]->domain, cpu) == NULL )
> +            return -ENOMEM;
> +    }

First: Two such if()s want folding.

>      else
>          idle_vcpu[cpu]->sched_unit->res = sr;
>  

Then: Down from here there is

    if ( idle_vcpu[cpu] == NULL )
        return -ENOMEM;

which your change is rendering redundant for at least the vcpu_create()
path.

Finally, as we're touching error handling here (and mayby more a question
to the maintainers than to you): What about sr in the error case? It's
being allocated earlier in the function, but not freed upon error. Hmm,
looks like cpu_schedule_down() is assumed to be taking care of the case,
yet then I wonder how that can assume that get_sched_res() would return
non-NULL - afaict it may be called without cpu_schedule_up() having run
first, or with it having bailed early with -ENOMEM.

Jan

Re: [XEN PATCH] common/sched: address a violation of MISRA C Rule 17.7

Posted by Jürgen Groß 2 months, 2 weeks ago

On 25.06.24 08:34, Jan Beulich wrote:
> On 25.06.2024 02:15, victorm.lira@amd.com wrote:
>> From: Victor Lira <victorm.lira@amd.com>
>>
>> Rule 17.7: "The value returned by a function having non-void return type
>> shall be used"
>>
>> This patch fixes this by adding a check to the return value.
>> No functional changes.
>>
>> Signed-off-by: Victor Lira <victorm.lira@amd.com>
>> ---
>> Cc: George Dunlap <george.dunlap@citrix.com>
>> Cc: Dario Faggioli <dfaggioli@suse.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: xen-devel@lists.xenproject.org
>> ---
>>   xen/common/sched/core.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index d84b65f197..e1cd824622 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -2789,7 +2789,10 @@ static int cpu_schedule_up(unsigned int cpu)
>>       BUG_ON(cpu >= NR_CPUS);
>>   
>>       if ( idle_vcpu[cpu] == NULL )
>> -        vcpu_create(idle_vcpu[0]->domain, cpu);
>> +    {
>> +        if ( vcpu_create(idle_vcpu[0]->domain, cpu) == NULL )
>> +            return -ENOMEM;
>> +    }
> 
> First: Two such if()s want folding.
> 
>>       else
>>           idle_vcpu[cpu]->sched_unit->res = sr;
>>   
> 
> Then: Down from here there is
> 
>      if ( idle_vcpu[cpu] == NULL )
>          return -ENOMEM;
> 
> which your change is rendering redundant for at least the vcpu_create()
> path.
> 
> Finally, as we're touching error handling here (and mayby more a question
> to the maintainers than to you): What about sr in the error case? It's
> being allocated earlier in the function, but not freed upon error. Hmm,
> looks like cpu_schedule_down() is assumed to be taking care of the case,
> yet then I wonder how that can assume that get_sched_res() would return
> non-NULL - afaict it may be called without cpu_schedule_up() having run
> first, or with it having bailed early with -ENOMEM.

Yes, you are right.

cpu_schedule_down() should bail out early in case sr is NULL.

I'll write a patch.


Juergen