[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/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 readabilityI 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)
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |