Re: [PATCH 5/5] x86: Fix missing brackets in macros

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2025-12-11 09:36, Jan Beulich wrote:
> On 10.12.2025 19:30, Andrew Cooper wrote:
>> With the wider testing, some more violations have been spotted.  This
>> addresses violations of Rule 20.7 which requires macro parameters to 
>> be
>> bracketed.
>> 
>> No functional change.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: consulting@bugseng.com <consulting@bugseng.com>
>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/arch/x86/mm/shadow/multi.c     | 2 +-
>>  xen/arch/x86/mm/shadow/private.h   | 6 +++---
>>  xen/drivers/passthrough/vtd/dmar.h | 2 +-
>>  xen/include/xen/kexec.h            | 4 ++--
>>  4 files changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/xen/arch/x86/mm/shadow/multi.c 
>> b/xen/arch/x86/mm/shadow/multi.c
>> index 03be61e225c0..36ee6554b4c4 100644
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -781,7 +781,7 @@ do {                                               
>>                      \
>>          (_sl1e) = _sp + _i;                                           
>>   \
>>          if ( shadow_l1e_get_flags(*(_sl1e)) & _PAGE_PRESENT )         
>>   \
>>              {_code}                                                   
>>   \
>> -        if ( _done ) break;                                           
>>   \
>> +        if ( (_done) ) break;                                         
>>   \
> 
> I don't understand this: There are parentheses already from if() 
> itself.
> 

Yeah, syntactically there are, but those are parsed as part of the if, 
rather than its condition; the AST node contained within does not have 
parentheses around it.

>> --- a/xen/arch/x86/mm/shadow/private.h
>> +++ b/xen/arch/x86/mm/shadow/private.h
>> @@ -636,9 +636,9 @@ prev_pinned_shadow(struct page_info *page,
>>  }
>> 
>>  #define foreach_pinned_shadow(dom, pos, tmp)                    \
>> -    for ( pos = prev_pinned_shadow(NULL, (dom));                \
>> -          pos ? (tmp = prev_pinned_shadow(pos, (dom)), 1) : 0;  \
>> -          pos = tmp )
>> +    for ( (pos) = prev_pinned_shadow(NULL, dom);                \
>> +          (pos) ? (tmp = prev_pinned_shadow(pos, dom), 1) : 0;  \
>> +          (pos) = tmp )
> 
> What about tmp (twice)?
> 
> Jan

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253