xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

[XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Federico Serafini 5 months, 3 weeks ago

Add missing parameter name "regs" and introduce function type
bug_fn_t: this improves readability and helps to validate that the
function passed to run_in_exception_handle() has the expected
prototype.
No functional change.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
Changes in v2:
  - adjusted tag;
  - avoided exceeding the 80-character limit.
---
 xen/arch/arm/traps.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ce89f16404..1264eab087 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
 
     if ( id == BUGFRAME_run_fn )
     {
-        void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
+        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);
+        bug_fn_t fn = (void *)regs->BUG_FN_REG;
 
         fn(regs);
         return 0;
-- 
2.34.1

Re: [XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Jan Beulich 5 months, 3 weeks ago

On 08.11.2023 16:42, Federico Serafini wrote:
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>  
>      if ( id == BUGFRAME_run_fn )
>      {
> -        void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);

Just to mention it: Type and name don't match here. You define a pointer-
to-function type, yet you name it as if it was a function type. Perhaps
the latter is really meant, such that ...

> +        bug_fn_t fn = (void *)regs->BUG_FN_REG;

... e.g. here the pointer-ness of the variable can still remain visible:

        bug_fn_t *fn = (void *)regs->BUG_FN_REG;

Jan

Re: [XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Julien Grall 5 months, 3 weeks ago

Hi,

On 08/11/2023 15:42, Federico Serafini wrote:
> Add missing parameter name "regs" and introduce function type
> bug_fn_t: this improves readability and helps to validate that the
> function passed to run_in_exception_handle() has the expected
> prototype.
> No functional change.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
> Changes in v2:
>    - adjusted tag;
>    - avoided exceeding the 80-character limit.
> ---
>   xen/arch/arm/traps.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ce89f16404..1264eab087 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>   
>       if ( id == BUGFRAME_run_fn )
>       {
> -        void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);

Thanks for sending a new version. This should be defined in asm/bug.h or 
maybe xen/bug.h as this is generic enough.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Federico Serafini 5 months, 3 weeks ago

On 08/11/23 17:04, Julien Grall wrote:
> Hi,
> 
> On 08/11/2023 15:42, Federico Serafini wrote:
>> Add missing parameter name "regs" and introduce function type
>> bug_fn_t: this improves readability and helps to validate that the
>> function passed to run_in_exception_handle() has the expected
>> prototype.
>> No functional change.
>>
>> Suggested-by: Julien Grall <julien@xen.org>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>> ---
>> Changes in v2:
>>    - adjusted tag;
>>    - avoided exceeding the 80-character limit.
>> ---
>>   xen/arch/arm/traps.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index ce89f16404..1264eab087 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs 
>> *regs, vaddr_t pc)
>>       if ( id == BUGFRAME_run_fn )
>>       {
>> -        void (*fn)(const struct cpu_user_regs *) = (void 
>> *)regs->BUG_FN_REG;
>> +        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);
> 
> Thanks for sending a new version. This should be defined in asm/bug.h or 
> maybe xen/bug.h as this is generic enough.

I see some uses of run_in_exception_handle() in common/bug.c and I guess
the type of the actual parameter should be changed to bug_fn_t.
Am I missing some other places where such change is needed?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Julien Grall 5 months, 3 weeks ago

Hi Federico,

On 08/11/2023 16:21, Federico Serafini wrote:
> On 08/11/23 17:04, Julien Grall wrote:
>> Hi,
>>
>> On 08/11/2023 15:42, Federico Serafini wrote:
>>> Add missing parameter name "regs" and introduce function type
>>> bug_fn_t: this improves readability and helps to validate that the
>>> function passed to run_in_exception_handle() has the expected
>>> prototype.
>>> No functional change.
>>>
>>> Suggested-by: Julien Grall <julien@xen.org>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>> ---
>>> Changes in v2:
>>>    - adjusted tag;
>>>    - avoided exceeding the 80-character limit.
>>> ---
>>>   xen/arch/arm/traps.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index ce89f16404..1264eab087 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs 
>>> *regs, vaddr_t pc)
>>>       if ( id == BUGFRAME_run_fn )
>>>       {
>>> -        void (*fn)(const struct cpu_user_regs *) = (void 
>>> *)regs->BUG_FN_REG;
>>> +        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);
>>
>> Thanks for sending a new version. This should be defined in asm/bug.h 
>> or maybe xen/bug.h as this is generic enough.
> 
> I see some uses of run_in_exception_handle() in common/bug.c and I guess
> the type of the actual parameter should be changed to bug_fn_t.
> Am I missing some other places where such change is needed?

Just to clarify, right now, I haven't suggested to replace all the 
open-coding version of the typedef. I am only asking to define the 
typedef in an header.

Note that run_in_exception_handler() is a macro. So you can't really 
change the type. But you could check that the type of the argument 
matches bug_fn_t.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2

Posted by Federico Serafini 5 months, 3 weeks ago

On 08/11/23 17:30, Julien Grall wrote:
> Hi Federico,
> 
> On 08/11/2023 16:21, Federico Serafini wrote:
>> On 08/11/23 17:04, Julien Grall wrote:
>>> Hi,
>>>
>>> On 08/11/2023 15:42, Federico Serafini wrote:
>>>> Add missing parameter name "regs" and introduce function type
>>>> bug_fn_t: this improves readability and helps to validate that the
>>>> function passed to run_in_exception_handle() has the expected
>>>> prototype.
>>>> No functional change.
>>>>
>>>> Suggested-by: Julien Grall <julien@xen.org>
>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>>> ---
>>>> Changes in v2:
>>>>    - adjusted tag;
>>>>    - avoided exceeding the 80-character limit.
>>>> ---
>>>>   xen/arch/arm/traps.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index ce89f16404..1264eab087 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1236,7 +1236,8 @@ int do_bug_frame(const struct cpu_user_regs 
>>>> *regs, vaddr_t pc)
>>>>       if ( id == BUGFRAME_run_fn )
>>>>       {
>>>> -        void (*fn)(const struct cpu_user_regs *) = (void 
>>>> *)regs->BUG_FN_REG;
>>>> +        typedef void (*bug_fn_t)(const struct cpu_user_regs *regs);
>>>
>>> Thanks for sending a new version. This should be defined in asm/bug.h 
>>> or maybe xen/bug.h as this is generic enough.
>>
>> I see some uses of run_in_exception_handle() in common/bug.c and I guess
>> the type of the actual parameter should be changed to bug_fn_t.
>> Am I missing some other places where such change is needed?
> 
> Just to clarify, right now, I haven't suggested to replace all the 
> open-coding version of the typedef. I am only asking to define the 
> typedef in an header.
OK.

> Note that run_in_exception_handler() is a macro. So you can't really 
> change the type. But you could check that the type of the argument 
> matches bug_fn_t.

I used the wrong terminology but this is what I meant.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)