Refactor the code to improve readability 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").

No functional change.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/common/efi/runtime.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index d03e5c90ce..acf08dcaa3 100644
--- 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);
+
         if ( !efi_fw_vendor )
             return -EOPNOTSUPP;
+
         info->vendor.revision = efi_fw_revision;
         n = info->vendor.bufsz / sizeof(*efi_fw_vendor);
-        if ( !guest_handle_okay(guest_handle_cast(info->vendor.name,
-                                                  CHAR16), n) )
+        if ( !guest_handle_okay(vendor_name, n) )
             return -EFAULT;
+
         for ( i = 0; i < n; ++i )
         {
             if ( __copy_to_guest_offset(info->vendor.name, i,
@@ -267,6 +273,8 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
                 break;
         }
         break;
+    }
+
     case XEN_FW_EFI_MEM_INFO:
         for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
         {
-- 
2.34.1

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.

Jan

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

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

On 2024-09-11 16:10, 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.
> 

Looking at the fully preprocessed code [1], there is an assignment to 
CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering 
the violation when used in such a way to be inside the sizeof().

if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode 
& ((1 << 4) << 10))) || (
__builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = 
(__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; 
}))._)))),1) && ((unsigned long)((unsigned long)((void *)(
full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
*)(full_ptr_t)(info->vendor.name).c; (
__compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * 
(sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; 
(__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
* (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
*)(full_ptr_t)(info->vendor.name).c; (
__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < 
((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
) )

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/latest/PROJECT.ecd;/by_service/MC3R1.R13.6.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":false,"selector":{"enabled":true,"negated":true,"kind":0,"domain":"message","inputs":[{"enabled":true,"text":"^.*xen/common/efi/runtime\\.c:258\\.15-258\\.31: 
`sizeof' expression trait"}]}}}

>> Wasn't there some other MISRA rule about lowercase/uppercase for macro 
>> names?
> 

There isn't one imposing this restriction (at least in MISRA C:2012, I 
haven't checked earlier editions).

> I can't recall having run into one, but I also haven't memorized them 
> all.
> 
> Jan

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

On 11.09.2024 16:27, Nicola Vetrini wrote:
> On 2024-09-11 16:10, 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.
>>
> 
> Looking at the fully preprocessed code [1], there is an assignment to 
> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering 
> the violation when used in such a way to be inside the sizeof().

I can see a number of initializers, but no assignment.

Jan

> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode 
> & ((1 << 4) << 10))) || (
> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = 
> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; 
> }))._)))),1) && ((unsigned long)((unsigned long)((void *)(
> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * 
> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; 
> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) 
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < 
> ((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
> ) )

On 11/09/24 16:57, Jan Beulich wrote:
> On 11.09.2024 16:27, Nicola Vetrini wrote:
>> On 2024-09-11 16:10, 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.
>>>
>>
>> Looking at the fully preprocessed code [1], there is an assignment to
>> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering
>> the violation when used in such a way to be inside the sizeof().
> 
> I can see a number of initializers, but no assignment.

+ Stefano in CC.

MISRA considers the initialization (even of a local variable)
a side effect. This is the reason of the violations.

I will send a V2 with a better description.

> 
>> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode
>> & ((1 << 4) << 10))) || (
>> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x =
>> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
>> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x };
>> }))._)))),1) && ((unsigned long)((unsigned long)((void *)(
>> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
>> *)(full_ptr_t)(info->vendor.name).c; (
>> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) *
>> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
>> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c;
>> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
>> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
>> *)(full_ptr_t)(info->vendor.name).c; (
>> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) <
>> ((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
>> ) )
> 

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

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

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