| Message ID | acbf40ba94a5ef7a8a429498765932b801e42a0a.1712305581.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C Rule 16.2 | expand |
On 05.04.2024 11:14, Nicola Vetrini wrote: > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -169,20 +169,22 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > > switch ( desc->Type ) > { > + default: > + type = E820_RESERVED; > + break; This one I guess is tolerable duplication-wise, and I might guess others would even have preferred it like that from the beginning. A blank line below here would be nice, though (and at some point ahead of and between the two ACPI- related case labels blank lines would want adding, too). However, ... > case EfiBootServicesCode: > case EfiBootServicesData: > if ( map_bs ) > { > - default: > type = E820_RESERVED; > break; > } > - /* fall through */ > + fallthrough; > case EfiConventionalMemory: > if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && > len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) > cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; > - /* fall through */ > + fallthrough; > case EfiLoaderCode: > case EfiLoaderData: > if ( desc->Attribute & EFI_MEMORY_RUNTIME ) ... below here there is else case EfiUnusableMemory: type = E820_UNUSABLE; break; I understand there are no figure braces here, and hence be the letter this isn't an issue with the Misra rule. But (a) some (e.g. Andrew, I guess) would likely argue for there wanting to be braces and (b) we don't want to be leaving this as is, when the spirit of the rule still suggests it should be done differently. Jan
On Mon, 8 Apr 2024, Jan Beulich wrote: > On 05.04.2024 11:14, Nicola Vetrini wrote: > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -169,20 +169,22 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > > > > switch ( desc->Type ) > > { > > + default: > > + type = E820_RESERVED; > > + break; > > This one I guess is tolerable duplication-wise, and I might guess others would > even have preferred it like that from the beginning. A blank line below here > would be nice, though (and at some point ahead of and between the two ACPI- > related case labels blank lines would want adding, too). > > However, ... > > > case EfiBootServicesCode: > > case EfiBootServicesData: > > if ( map_bs ) > > { > > - default: > > type = E820_RESERVED; > > break; > > } > > - /* fall through */ > > + fallthrough; > > case EfiConventionalMemory: > > if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && > > len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) > > cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; > > - /* fall through */ > > + fallthrough; > > case EfiLoaderCode: > > case EfiLoaderData: > > if ( desc->Attribute & EFI_MEMORY_RUNTIME ) > > ... below here there is > > else > case EfiUnusableMemory: > type = E820_UNUSABLE; > break; > > I understand there are no figure braces here, and hence be the letter this > isn't an issue with the Misra rule. But (a) some (e.g. Andrew, I guess) > would likely argue for there wanting to be braces and (b) we don't want to > be leaving this as is, when the spirit of the rule still suggests it should > be done differently. I agree. For clarity I would go with the following because it is easier to read: case EfiLoaderData: if ( desc->Attribute & EFI_MEMORY_RUNTIME ) type = E820_RESERVED; else if ( desc->Attribute & EFI_MEMORY_WB ) type = E820_RAM; else type = E820_UNUSABLE; break; case EfiUnusableMemory: type = E820_UNUSABLE; break;
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 8ea64e31cdc2..c4d452c482be 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -169,20 +169,22 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, switch ( desc->Type ) { + default: + type = E820_RESERVED; + break; case EfiBootServicesCode: case EfiBootServicesData: if ( map_bs ) { - default: type = E820_RESERVED; break; } - /* fall through */ + fallthrough; case EfiConventionalMemory: if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; - /* fall through */ + fallthrough; case EfiLoaderCode: case EfiLoaderData: if ( desc->Attribute & EFI_MEMORY_RUNTIME )
Refactor the first clauses so that a violation of MISRA C Rule 16.2 is resolved (a switch label, "default" in this case, should be immediately enclosed in the compound statement of the switch). Note that the switch clause ending with the pseudo keyword "fallthrough" is an allowed exception to Rule 16.3. Convert fallthrough comments in other clauses to the pseudo-keyword while at it. No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- There is some degree of duplication here between the default clause and the others, but I don't think there is a way to avoid it. --- xen/arch/x86/efi/efi-boot.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)