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.
~Andrew
