Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

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

On 2024-02-29 17:25, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/include/xen/kconfig.h
>> +++ b/xen/include/xen/kconfig.h
>> @@ -25,7 +25,7 @@
>>  #define __ARG_PLACEHOLDER_1 0,
>>  #define config_enabled(cfg) _config_enabled(cfg)
>>  #define _config_enabled(value) 
>> __config_enabled(__ARG_PLACEHOLDER_##value)
>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> 1, 0)
>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> (1), (0))
>>  #define ___config_enabled(__ignored, val, ...) val
> 
> In addition to what Andrew said, would you mind clarifying what exactly 
> the
> violation is here? I find it questionable that numeric literals need
> parenthesizing; they don't normally need to, aynwhere.
> 

This one's a little special. I couldn't parenthesize val further down, 
because then it would break the build:

In file included from ././include/xen/config.h:14,
                  from <command-line>:
./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
‘)’ token
    29 | #define ___config_enabled(__ignored, val, ...) (val)
       |                                                    ^
./include/xen/kconfig.h:39:34: note: in expansion of macro 
‘___config_enabled’
    39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
static,)
       |                                  ^~~~~~~~~~~~~~~~~
./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
    38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
       |                          ^~~~~~~~~~
./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
    41 | #define STATIC_IF(option) static_if(option)
       |                           ^~~~~~~~~
common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
   260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
INVALID_MFN_INITIALIZER;
       | ^~~~~~~~~
make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1


>> --- a/xen/include/xen/list.h
>> +++ b/xen/include/xen/list.h
>> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
>> list_head *list,
>>   * @member: the name of the list_struct within the struct.
>>   */
>>  #define list_for_each_entry(pos, head, member)                        
>>   \
>> -    for (pos = list_entry((head)->next, typeof(*pos), member);        
>>   \
>> -         &pos->member != (head);                                      
>>   \
>> -         pos = list_entry(pos->member.next, typeof(*pos), member))
>> +    for (pos = list_entry((head)->next, typeof(*(pos)), member);      
>>     \
>> +         &(pos)->member != (head);                                    
>>   \
>> +         pos = list_entry((pos)->member.next, typeof(*(pos)), 
>> member))
> 
> this ends up inconsistent, which I think isn't nice: Some uses of "pos"
> are now parenthesized, while others aren't. Applies further down as 
> well.
> 

Yeah, the inconsistency is due to the fact that a non-parenthesized 
parameter as lhs is already deviated. To keep it consistent here I can 
add parentheses, but then the deviation would be kind of pointless, 
wouldn't it?

> You may also want to take this as a strong suggestion to split 
> dissimilar
> changes, so uncontroversial parts can go in.
> 

Ok, that was an oversight.

>> @@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct 
>> hlist_node *prev,
>>            pos = pos->next)
>> 
>>  #endif /* __XEN_LIST_H__ */
>> -
> 
> Unrelated change?
> 

Oh, yes. Will drop it.

>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -94,7 +94,7 @@ struct lock_profile_qhead {
>>      int32_t                   idx;     /* index for printout */
>>  };
>> 
>> -#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
>> &lockname, }
>> +#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
>> &(lockname), }
> 
> This also may be viewed as falling in the same category, but is less
> problematic because the other use is stringification, when in principle
> some kind of expression would be passed in (albeit in practice I don't
> expect anyone would do that).
> 

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