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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2024 16:27, Andrew Cooper wrote:
> On 11/09/2024 3:10 pm, Jan Beulich wrote:
>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>>> 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?
>> If Eclair didn't look at the macro-expanded code, it wouldn't even see
>> the sizeof(). Hence I don't expect the thing to be mistaken for a function
>> call.
> 
> The complaint is a sizeof in guest_handle_okay() being given ({ ... })
> to interpret.
> 
> ({}) can have arbitrary side effects in it, hence the violation.

I sincerely hope the tool actually looks inside the ({}).

Jan