Re: [RFC PATCH] x86/vlapic: address a violation of MISRA C:2012 Rule 16.2

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2023 15:22, Nicola Vetrini wrote:
> The clauses of a switch should be enclosed directly by a switch
> statement to make the code more easily understandable and less
> prone to errors.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
> This patch is mainly indended to probe how the community, especially the
> maintainers, would receive such modifications to the code, and whether there
> would be consensus on the rule's adoption. Anyone is welcome to
> give feedback on this, especially on the x86 side, where this pattern
> is used more frequently.

The chosen instance (below) is one where it is relatively easy to argue
that putting the default label in an inner scope isn't much of a
difference as far as overall code size / redundancy is concerned. But
there are (perhaps many) other cases where the gains of using what
Misra dislikes are much higher.

This is another one of the various more recently discussed rules where
I think Misra is just going too far, dictating various aspects of style
for - in my personal view - no real gain. Furthermore, if you could
pick some more involved example (arch/x86/x86_emulate/x86_emulate.c may
yield a few "good" examples), I'd like to learn how you propose to
change such code, with two up-front constraints:
- no added redundancy,
- no new goto.
Either of them in replacement code would go against what the description
above states as a goal.

Jan

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1034,10 +1034,10 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, 
> uint64_t val)
>      case APIC_EOI:
>      case APIC_ESR:
>          if ( val )
> -        {
> -    default:
>              return X86EMUL_EXCEPTION;
> -        }
> +        break;
> +    default:
> +        return X86EMUL_EXCEPTION;
>      }
>  
>      vlapic_reg_write(v, array_index_nospec(offset, PAGE_SIZE), val);