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.
> Wasn't there some other MISRA rule about lowercase/uppercase for macro names?
I can't recall having run into one, but I also haven't memorized them all.
Jan
