Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
> On 10.09.2024 21:06, Federico Serafini wrote:
> > Refactor the code to improve readability
> 
> I question this aspect. I'm not the maintainer of this code anymore, so
> my view probably doesn't matter much here.
> 
> > and address violations of
> > MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
> > not contain any expression which has potential side effect").
> 
> Where's the potential side effect? Since you move ...
> 
> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info 
> > *info)
> >          info->cfg.addr = __pa(efi_ct);
> >          info->cfg.nent = efi_num_ct;
> >          break;
> > +
> >      case XEN_FW_EFI_VENDOR:
> > +    {
> > +        XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
> > +            guest_handle_cast(info->vendor.name, CHAR16);
> 
> .. this out, it must be the one. I've looked at it, yet I can't spot
> anything:
> 
> #define guest_handle_cast(hnd, type) ({         \
>     type *_x = (hnd).p;                         \
>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
> })
> 
> As a rule of thumb, when things aren't obvious, please call out the
> specific aspect / property in descriptions of such patches.

I guess it's because guest_handle_cast() is a macro, yet it's lowercase
so looks like a function? Wasn't there some other MISRA rule about
lowercase/uppercase for macro names?

And yes, I don't really see why this would violate the side effect rule
either.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature