| Message ID | 24c2e8b7a7becc6b16d0b37f2c642a9b9e976269.1699457659.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v2] xen/arm: traps: address a violation of MISRA C:2012 Rule 8.2 | expand |
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,
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?
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,
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.
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
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;
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(-)