Re: [XEN PATCH v2 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2023 09:33, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The renames done by this patch avoid shadowing from happening.
> They are as follows:
> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>   'x86_mcinfo_lookup' that defines 'i' as a loop counter)

Then that macro needs adjustment. Especially considering it's in a
public header: It's full of name space violations.

> --- a/xen/arch/x86/include/asm/hvm/save.h
> +++ b/xen/arch/x86/include/asm/hvm/save.h
> @@ -49,21 +49,21 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>   */
>  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
>      int r;                                                              \
> -    struct hvm_save_descriptor *desc                                    \
> +    struct hvm_save_descriptor *descriptor                              \
>          = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
>      if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
>                 HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
>      {                                                                   \
>          _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
>          if ( HVM_SAVE_HAS_COMPAT(_x) &&                                 \
> -             desc->length != HVM_SAVE_LENGTH(_x) )                      \
> -            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);          \
> +             descriptor->length != HVM_SAVE_LENGTH(_x) )                \
> +            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);    \
>      }                                                                   \
>      else if (HVM_SAVE_HAS_COMPAT(_x)                                    \
>               && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
>                         HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
>          _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
> -        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);              \
> +        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);        \
>      }                                                                   \
>      r; })

The macro-local variable gets too long for my taste, to be honest,
and it being improperly named anyway suggests it simply wants a
trailing underscore added. And then, since for a variable named "r"
the risk of shadowing is equally high, that one wants to gain a
trailing underscore as well imo. (And while at it, I personally
would also drop the leading underscores from the macro parameter
names. Furthermore I think it would be nice if at on the lines
touched anyway indentation was also corrected. Overall maybe best
if I submit a patch.)

Jan