| Message ID | cover.1710145041.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xen: address violation of MISRA C:2012 Directive 4.10 | expand |
On 11.03.2024 09:59, Simone Ballarin wrote: > The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states: > "Precautions shall be taken in order to prevent the contents of a header file > being included more than once". > > As stated in v2, the following naming convention has been estabilished: > - arch/.../include/asm/ headers -> ASM_<filename>_H > - private headers -> <dir>_<filename>_H > - asm-generic headers -> ASM_GENERIC_<filename>_H > > Since there would have been conflicting guards between architectures (which is a violation > of the directive), Why would this be a violation? The two sets of headers aren't use together, and any scan should only point out collisions in what's actively used, I'd hope. > there has been a need for ASM headers to specify if the inclusion guard > referred to ARM or X86. Hence it has been decided to adopt instead: > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H > > The subdir used is the smallest possible to avoid collisions. For example, it has been > observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there > are plenty of header files with the same name, hence _ARMxx_ was added as subdirectory. Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get away with one less name component. > There has been a need to define a standard for generated headers too: > - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H > - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H > > To summarize, here are all the rules that have been applied: > - private headers -> <dir>_<filename>_H And <dir> here is the full path? I thought I had indicated before that this is going to lead to excessively long identifiers. > - asm-generic headers -> ASM_GENERIC_<filename>_H > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H > - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H > - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which doesn't seem very likely)? Jan
On 11/03/24 10:59, Jan Beulich wrote: > On 11.03.2024 09:59, Simone Ballarin wrote: >> The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states: >> "Precautions shall be taken in order to prevent the contents of a header file >> being included more than once". >> >> As stated in v2, the following naming convention has been estabilished: >> - arch/.../include/asm/ headers -> ASM_<filename>_H >> - private headers -> <dir>_<filename>_H >> - asm-generic headers -> ASM_GENERIC_<filename>_H >> >> Since there would have been conflicting guards between architectures (which is a violation >> of the directive), > > Why would this be a violation? The two sets of headers aren't use together, > and any scan should only point out collisions in what's actively used, I'd > hope. > A header guard cannot be used multiple times in the same project, so it's a question of whether or not we want to consider each arch variant part of the same project. In case, we can define a new rule for these files. >> there has been a need for ASM headers to specify if the inclusion guard >> referred to ARM or X86. Hence it has been decided to adopt instead: >> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H >> >> The subdir used is the smallest possible to avoid collisions. For example, it has been >> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there >> are plenty of header files with the same name, hence _ARMxx_ was added as subdirectory. > > Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get > away with one less name component. > I agree. >> There has been a need to define a standard for generated headers too: >> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H >> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H >> >> To summarize, here are all the rules that have been applied: >> - private headers -> <dir>_<filename>_H > > And <dir> here is the full path? I thought I had indicated before that > this is going to lead to excessively long identifiers. > Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply. If this generates excessively long guards, we can simply add more rules. >> - asm-generic headers -> ASM_GENERIC_<filename>_H >> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H >> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H >> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H > > And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which > doesn't seem very likely)? > Yes, it is. Do you prefer dropping the _ARM_? > Jan
On 11.03.2024 12:41, Simone Ballarin wrote: > On 11/03/24 10:59, Jan Beulich wrote: >> On 11.03.2024 09:59, Simone Ballarin wrote: >>> The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states: >>> "Precautions shall be taken in order to prevent the contents of a header file >>> being included more than once". >>> >>> As stated in v2, the following naming convention has been estabilished: >>> - arch/.../include/asm/ headers -> ASM_<filename>_H >>> - private headers -> <dir>_<filename>_H >>> - asm-generic headers -> ASM_GENERIC_<filename>_H >>> >>> Since there would have been conflicting guards between architectures (which is a violation >>> of the directive), >> >> Why would this be a violation? The two sets of headers aren't use together, >> and any scan should only point out collisions in what's actively used, I'd >> hope. >> > > A header guard cannot be used multiple times in the same project, so it's a question > of whether or not we want to consider each arch variant part of the same project. > In case, we can define a new rule for these files. > >>> there has been a need for ASM headers to specify if the inclusion guard >>> referred to ARM or X86. Hence it has been decided to adopt instead: >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H >>> >>> The subdir used is the smallest possible to avoid collisions. For example, it has been >>> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there >>> are plenty of header files with the same name, hence _ARMxx_ was added as subdirectory. >> >> Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get >> away with one less name component. >> > > I agree. > >>> There has been a need to define a standard for generated headers too: >>> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H >>> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H >>> >>> To summarize, here are all the rules that have been applied: >>> - private headers -> <dir>_<filename>_H >> >> And <dir> here is the full path? I thought I had indicated before that >> this is going to lead to excessively long identifiers. >> > > Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply. > If this generates excessively long guards, we can simply add more rules. >>> - asm-generic headers -> ASM_GENERIC_<filename>_H >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H >>> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H >>> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H >> >> And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which >> doesn't seem very likely)? > > Yes, it is. Do you prefer dropping the _ARM_? Well, I prefer any measure keeping down the length of these identifiers. Jan