[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6
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))) > ) )
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |