Re: [XEN PATCH v2 2/3] xen/gnttab: address a violation of MISRA C Rule 13.6

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 30 Sep 2024, Federico Serafini wrote:
> guest_handle_ok()'s expansion contains a sizeof() involving its
> first argument guest_handle_cast().
> The expansion of the latter, in turn, contains a variable
> initialization.
> 
> Since MISRA considers the initialization (even of a local variable)
> a side effect, the chain of expansions mentioned above violates
> MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator shall not
> contain any expression which has potential side effect).
> 
> Refactor the code to address the rule violation.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>

There is a pending interesting comment from Jan on patch #1 that affects
this patch too, but I think this patch is good even just as a
readability improvement so I'll review it as is


> ---
> Changes in v2:
> - better description;
> - preserved original indentation.
> ---
>  xen/common/compat/grant_table.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
> index 5ad0debf96..bbb717bf64 100644
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -78,12 +78,15 @@ int compat_grant_table_op(
>          cmd_op = cmd;
>      switch ( cmd_op )
>      {
> -#define CASE(name) \
> -    case GNTTABOP_##name: \
> -        if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \
> -                                                           
> gnttab_##name##_compat_t), \
> -                                         count)) ) \
> -            rc = -EFAULT; \
> +#define CASE(name)                                                  \
> +    case GNTTABOP_ ## name:                                         \
> +    {                                                               \
> +        XEN_GUEST_HANDLE_PARAM(gnttab_ ## name ## _compat_t) h =    \
> +            guest_handle_cast(uop, gnttab_ ## name ## _compat_t);   \
> +                                                                    \
> +        if ( unlikely(!guest_handle_okay(h, count)) )               \
> +            rc = -EFAULT;                                           \
> +    }                                                               \
>          break

We would typically put the break within the case { }

Other than that, I think this. With that change:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



>  #ifndef CHECK_gnttab_map_grant_ref
> -- 
> 2.43.0
>