| Message ID | c2b0bb92a246e5cf149b1ec81c6ed635a275f9df.1691575243.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address MISRA C:2012 Rule 8.4 | expand |
On 09.08.2023 13:02, Nicola Vetrini wrote: > 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow > the declaration of 'arch_get_xen_caps' to be visible when > defining the function. > > The header 'xen/delay.h' is included in 'xen/arch/arm/time.c' > to allow the declaration of 'udelay' to be visible. > > At the same time, a declaration for 'get_sec' is added in > 'xen/include/xen/time.h' to be available for every call site > (in particular cper.h). > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/arch/arm/setup.c | 1 + > xen/arch/arm/time.c | 1 + > xen/include/xen/cper.h | 3 +-- > xen/include/xen/time.h | 1 + > 4 files changed, 4 insertions(+), 2 deletions(-) I would have almost put this off as Arm-only, but then saw this diffstat. How come you consider cper.h Arm-related? This is used only by APEI source files, which in turn are used only by x86 afaics. So I think you want to split off the movement of the get_sec() declaration. As to title and description (perhaps affecting more than just this patch): Failing to have declarations in scope where definitions appear is an at least latent bug. We fix these as we find them anyway, so Misra is secondary here. Hence I'd like to suggest that you declare any such change as an actual bugfix, ideally including a Fixes: tag. It is of course fine to mention that this then also addresses Misra rule 8.4. Jan
> On 9 Aug 2023, at 13:42, Jan Beulich <jbeulich@suse.com> wrote: > > On 09.08.2023 13:02, Nicola Vetrini wrote: >> 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow >> the declaration of 'arch_get_xen_caps' to be visible when >> defining the function. >> >> The header 'xen/delay.h' is included in 'xen/arch/arm/time.c' >> to allow the declaration of 'udelay' to be visible. >> >> At the same time, a declaration for 'get_sec' is added in >> 'xen/include/xen/time.h' to be available for every call site >> (in particular cper.h). >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/arch/arm/setup.c | 1 + >> xen/arch/arm/time.c | 1 + >> xen/include/xen/cper.h | 3 +-- >> xen/include/xen/time.h | 1 + >> 4 files changed, 4 insertions(+), 2 deletions(-) > > I would have almost put this off as Arm-only, but then saw this diffstat. > How come you consider cper.h Arm-related? This is used only by APEI source > files, which in turn are used only by x86 afaics. So I think you want to > split off the movement of the get_sec() declaration. > > As to title and description (perhaps affecting more than just this patch): > Failing to have declarations in scope where definitions appear is an at > least latent bug. We fix these as we find them anyway, so Misra is > secondary here. Hence I'd like to suggest that you declare any such > change as an actual bugfix, ideally including a Fixes: tag. To prevent back and forth I would suggest also to have a look in sending-patches.pandoc, ### Fixes section, on the expected syntax for the tag > It is of > course fine to mention that this then also addresses Misra rule 8.4. > > Jan >
On 09/08/2023 14:42, Jan Beulich wrote: > On 09.08.2023 13:02, Nicola Vetrini wrote: >> 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow >> the declaration of 'arch_get_xen_caps' to be visible when >> defining the function. >> >> The header 'xen/delay.h' is included in 'xen/arch/arm/time.c' >> to allow the declaration of 'udelay' to be visible. >> >> At the same time, a declaration for 'get_sec' is added in >> 'xen/include/xen/time.h' to be available for every call site >> (in particular cper.h). >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/arch/arm/setup.c | 1 + >> xen/arch/arm/time.c | 1 + >> xen/include/xen/cper.h | 3 +-- >> xen/include/xen/time.h | 1 + >> 4 files changed, 4 insertions(+), 2 deletions(-) > > I would have almost put this off as Arm-only, but then saw this > diffstat. > How come you consider cper.h Arm-related? This is used only by APEI > source > files, which in turn are used only by x86 afaics. So I think you want > to > split off the movement of the get_sec() declaration. > My mistake, I squashed the arm and the cper part together to avoid touching the time headers twice, but the old tag remained. > As to title and description (perhaps affecting more than just this > patch): > Failing to have declarations in scope where definitions appear is an at > least latent bug. We fix these as we find them anyway, so Misra is > secondary here. Hence I'd like to suggest that you declare any such > change as an actual bugfix, ideally including a Fixes: tag. It is of > course fine to mention that this then also addresses Misra rule 8.4. > > Jan Ok
> To prevent back and forth I would suggest also to have a look in > sending-patches.pandoc, > ### Fixes section, on the expected syntax for the tag > Thanks
On Wed, 9 Aug 2023, Jan Beulich wrote: > On 09.08.2023 13:02, Nicola Vetrini wrote: > > 'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow > > the declaration of 'arch_get_xen_caps' to be visible when > > defining the function. > > > > The header 'xen/delay.h' is included in 'xen/arch/arm/time.c' > > to allow the declaration of 'udelay' to be visible. > > > > At the same time, a declaration for 'get_sec' is added in > > 'xen/include/xen/time.h' to be available for every call site > > (in particular cper.h). > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > --- > > xen/arch/arm/setup.c | 1 + > > xen/arch/arm/time.c | 1 + > > xen/include/xen/cper.h | 3 +-- > > xen/include/xen/time.h | 1 + > > 4 files changed, 4 insertions(+), 2 deletions(-) > > I would have almost put this off as Arm-only, but then saw this diffstat. > How come you consider cper.h Arm-related? This is used only by APEI source > files, which in turn are used only by x86 afaics. So I think you want to > split off the movement of the get_sec() declaration. > > As to title and description (perhaps affecting more than just this patch): > Failing to have declarations in scope where definitions appear is an at > least latent bug. We fix these as we find them anyway, so Misra is > secondary here. Hence I'd like to suggest that you declare any such > change as an actual bugfix, ideally including a Fixes: tag. It is of > course fine to mention that this then also addresses Misra rule 8.4. As you split the patches moving cper.h out, and fixing the commit message, please add my Reviewed-by for the setup.c/time.c/time.h changes.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index bbf72b69aa..44ccea03ca 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -32,6 +32,7 @@ #include <xen/libfdt/libfdt-xen.h> #include <xen/acpi.h> #include <xen/warning.h> +#include <xen/hypercall.h> #include <asm/alternative.h> #include <asm/page.h> #include <asm/current.h> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index 0b482d7db3..3535bd8ac7 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -17,6 +17,7 @@ #include <xen/softirq.h> #include <xen/sched.h> #include <xen/time.h> +#include <xen/delay.h> #include <xen/sched.h> #include <xen/event.h> #include <xen/acpi.h> diff --git a/xen/include/xen/cper.h b/xen/include/xen/cper.h index 7c6a4c45ce..de8f385bdd 100644 --- a/xen/include/xen/cper.h +++ b/xen/include/xen/cper.h @@ -23,8 +23,7 @@ #include <xen/types.h> #include <xen/string.h> - -extern unsigned long get_sec(void); +#include <xen/time.h> typedef struct { uint8_t b[16]; diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h index 5aafdda4f3..67c586b736 100644 --- a/xen/include/xen/time.h +++ b/xen/include/xen/time.h @@ -36,6 +36,7 @@ s_time_t get_s_time_fixed(u64 at_tick); s_time_t get_s_time(void); unsigned long get_localtime(struct domain *d); uint64_t get_localtime_us(struct domain *d); +unsigned long get_sec(void); struct tm { int tm_sec; /* seconds */
'xen/hypercall.h' is included in 'xen/arch/arm/setup.c' to allow the declaration of 'arch_get_xen_caps' to be visible when defining the function. The header 'xen/delay.h' is included in 'xen/arch/arm/time.c' to allow the declaration of 'udelay' to be visible. At the same time, a declaration for 'get_sec' is added in 'xen/include/xen/time.h' to be available for every call site (in particular cper.h). Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/arm/setup.c | 1 + xen/arch/arm/time.c | 1 + xen/include/xen/cper.h | 3 +-- xen/include/xen/time.h | 1 + 4 files changed, 4 insertions(+), 2 deletions(-)