Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Jul 25, 2014, at 7:06 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:

> Il 25/07/2014 15:27, Jeff Kirsher ha scritto:
>> From: Mark Rustad <mark.d.rustad@xxxxxxxxx>
>> 
>> Resolve shadow warnings that appear in W=2 builds. In this case,
>> a macro declared an inner local variable with the same name as an
>> outer one. This can be a serious hazard in the event that the outer
>> variable is ever passed as a parameter, as the inner variable will
>> be referenced instead of what was intended. This macro doesn't have
>> any parameters - at this time - but prepend an _ to all of the
>> macro-declared variables as is the custom, to resolve the warnings
>> and eliminate any future hazard.
>> 
>> Signed-off-by: Mark Rustad <mark.d.rustad@xxxxxxxxx>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@xxxxxxxxx>
>> ---
>> arch/x86/kvm/mmutrace.h | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
>> index 9d2e0ff..2d8d00c 100644
>> --- a/arch/x86/kvm/mmutrace.h
>> +++ b/arch/x86/kvm/mmutrace.h
>> @@ -22,26 +22,26 @@
>> 	__entry->unsync = sp->unsync;
>> 
>> #define KVM_MMU_PAGE_PRINTK() ({				        \
>> -	const char *ret = p->buffer + p->len;				\
>> -	static const char *access_str[] = {			        \
>> +	const char *_ret = p->buffer + p->len;				\
>> +	static const char *_access_str[] = {			        \
>> 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
>> 	};							        \
>> -	union kvm_mmu_page_role role;				        \
>> +	union kvm_mmu_page_role _role;				        \
>> 								        \
>> -	role.word = __entry->role;					\
>> +	_role.word = __entry->role;					\
>> 									\
>> 	trace_seq_printf(p, "sp gen %lx gfn %llx %u%s q%u%s %s%s"	\
>> 			 " %snxe root %u %s%c",	__entry->mmu_valid_gen,	\
>> -			 __entry->gfn, role.level,			\
>> -			 role.cr4_pae ? " pae" : "",			\
>> -			 role.quadrant,					\
>> -			 role.direct ? " direct" : "",			\
>> -			 access_str[role.access],			\
>> -			 role.invalid ? " invalid" : "",		\
>> -			 role.nxe ? "" : "!",				\
>> +			 __entry->gfn, _role.level,			\
>> +			 _role.cr4_pae ? " pae" : "",			\
>> +			 _role.quadrant,				\
>> +			 _role.direct ? " direct" : "",			\
>> +			 _access_str[_role.access],			\
>> +			 _role.invalid ? " invalid" : "",		\
>> +			 _role.nxe ? "" : "!",				\
>> 			 __entry->root_count,				\
>> 			 __entry->unsync ? "unsync" : "sync", 0);	\
>> -	ret;								\
>> +	_ret;								\
>> 		})
>> 
>> #define kvm_mmu_trace_pferr_flags       \
>> 
> 
> I think this unnecessarily uglifies the code, so I am not applying it.
> Gleb, what do you think?

Would you consider a version that only changes ret to _ret? That would be enough to resolve the warning. I only did the other variables because it seemed to be a best practice for these inner block declarations.

Hmmm. It seems like p should really be a parameter to this macro.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux