Re: Address MISRA C:2012 Rule 8.4

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 4 Aug 2023, Jan Beulich wrote:
> On 04.08.2023 11:47, Nicola Vetrini wrote:
> > Upon further examination, I identified the following patterns:
> > 
> > 1. Functions defined in .c called only from asm code (e.g., the already 
> > mentioned __start_xen)
> > 2. Functions/variables declared in a .h, defined in a .c that does not 
> > include the .h with the declaration
> > (e.g., 'fill_console_start_info' is defined in 'xen/drivers/vga.c', 
> > declared in 'xen/include/xen/console.h' which is not visible when 
> > compiling the .c).
> > 3. Variables that are either extern or not, such as 'acpi_gbl_FADT' in 
> > 'xen/include/acpi/acglobal.h', depending on
> >     DEFINE_ACPI_GLOBALS
> > 
> > Below are the proposed resolution strategies:
> > 
> > 1. I would advise to add the declaration in the relative .h, to support 
> > automatic consistency checks with the
> >     implementation and a quick reference when touching the asm.
> 
> That'll need discussing first.

My take is that it would be nicer if they were declared in .h files. It
would also be useful for ourselves as documentation/reference. So I
would be OK accepting patches like that. But at the same time I don't
see it as a must-have requirement for safety (the .h declaration would
not be actually used).


> > 2. To comply with the rule, the header with the declaration should be 
> > included. Also note that there are some
> >     corner cases, such as 'get_sec', which is used in 'cper.h' without 
> > including 'time.h' (which should gain a
> >     declaration for it).
> 
> This one of course wants fixing wherever found.

+1


> > 3. One possible resolution pattern is including 'acglobal.h' twice 
> > (either directly or indirectly trough acpi.h, if
> >     the latter does not cause other issues) like so:
> > 
> >     (assuming DEFINE_ACPI_GLOBALS is undefined here)
> >     #include "acglobal.h"
> >     #define DEFINE_ACPI_GLOBALS
> >     #include  "acglobal.h"
> > 
> >    this way, the rule is followed properly, though it's not the prettiest 
> > pattern and also clashes with the objectives
> >    of D4.10 ("Precautions shall be taken in order to prevent the contents 
> > of a header file being included
> >    more than once"), but then a motivated exception is allowed there.
> 
> Not really sure about this one.
 
I would leave these alone because it was clearly done on purpose "to
simplify maintenance of the code" as the in-code comment claims. Also
this is very old code, probably inherited from somewhere else and never
changed.