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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)