[XEN,v2,3/7] xen/list: address violations of MISRA C Rule 20.7

Commit Message

Nicola Vetrini March 8, 2024, 11:21 a.m. UTC

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <[e-mail]>
---
Changes in v2:
- changes to list.h are all in this patch;
- Parenthesized some instances of "pos" and "n" even when already
covered by the deviation on the lhs of an assingment for consistency.
---
 xen/include/xen/list.h | 83 +++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 42 deletions(-)

Comments

Stefano Stabellini March 9, 2024, 1:29 a.m. UTC | #1

On Fri, 8 Mar 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <[e-mail]>

Reviewed-by: Stefano Stabellini <[e-mail]>

Jan Beulich March 11, 2024, 7:48 a.m. UTC | #2

On 08.03.2024 12:21, Nicola Vetrini wrote:
> --- a/xen/include/xen/list.h
> +++ b/xen/include/xen/list.h
> @@ -479,9 +479,9 @@ static inline void list_splice_init(struct list_head *list,
>   * @n:      another &struct list_head to use as temporary storage
>   * @head:   the head for your list.
>   */
> -#define list_for_each_backwards_safe(pos, n, head)              \
> -    for ( pos = (head)->prev, n = pos->prev; pos != (head);     \
> -          pos = n, n = pos->prev )
> +#define list_for_each_backwards_safe(pos, n, head)                 \
> +    for ( (pos) = (head)->prev, n = (pos)->prev; (pos) != (head);  \
> +          (pos) = n, n = (pos)->prev )

I think this is worse than before and incomplete: At least the rhs use of n
also needs parenthesizing. Plus if pos is parenthesized even in lhs
instances, imo n ought to be, too.

Jan

Nicola Vetrini March 11, 2024, 8:19 a.m. UTC | #3

On 2024-03-11 08:48, Jan Beulich wrote:
> On 08.03.2024 12:21, Nicola Vetrini wrote:
>> --- a/xen/include/xen/list.h
>> +++ b/xen/include/xen/list.h
>> @@ -479,9 +479,9 @@ static inline void list_splice_init(struct 
>> list_head *list,
>>   * @n:      another &struct list_head to use as temporary storage
>>   * @head:   the head for your list.
>>   */
>> -#define list_for_each_backwards_safe(pos, n, head)              \
>> -    for ( pos = (head)->prev, n = pos->prev; pos != (head);     \
>> -          pos = n, n = pos->prev )
>> +#define list_for_each_backwards_safe(pos, n, head)                 \
>> +    for ( (pos) = (head)->prev, n = (pos)->prev; (pos) != (head);  \
>> +          (pos) = n, n = (pos)->prev )
> 
> I think this is worse than before and incomplete: At least the rhs use 
> of n
> also needs parenthesizing. Plus if pos is parenthesized even in lhs
> instances, imo n ought to be, too.
> 

You are right: this was simply an oversight.

Patch

diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
index b5eab3a1eb6c..27cafcce2c82 100644
--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -479,9 +479,9 @@  static inline void list_splice_init(struct list_head *list,
  * @n:      another &struct list_head to use as temporary storage
  * @head:   the head for your list.
  */
-#define list_for_each_backwards_safe(pos, n, head)              \
-    for ( pos = (head)->prev, n = pos->prev; pos != (head);     \
-          pos = n, n = pos->prev )
+#define list_for_each_backwards_safe(pos, n, head)                 \
+    for ( (pos) = (head)->prev, n = (pos)->prev; (pos) != (head);  \
+          (pos) = n, n = (pos)->prev )
 
 /**
  * list_for_each_entry - iterate over list of given type
@@ -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))
 
 /**
  * list_for_each_entry_reverse - iterate backwards over list of given type.
@@ -501,9 +501,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_reverse(pos, head, member)                  \
-    for (pos = list_entry((head)->prev, typeof(*pos), member);          \
-         &pos->member != (head);                                        \
-         pos = list_entry(pos->member.prev, typeof(*pos), member))
+    for ((pos) = list_entry((head)->prev, typeof(*(pos)), member);      \
+         &(pos)->member != (head);                                      \
+         (pos) = list_entry((pos)->member.prev, typeof(*(pos)), member))
 
 /**
  * list_prepare_entry - prepare a pos entry for use in
@@ -516,7 +516,7 @@  static inline void list_splice_init(struct list_head *list,
  * list_for_each_entry_continue.
  */
 #define list_prepare_entry(pos, head, member)           \
-    ((pos) ? : list_entry(head, typeof(*pos), member))
+    ((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -527,10 +527,10 @@  static inline void list_splice_init(struct list_head *list,
  * Continue to iterate over list of given type, continuing after
  * the current position.
  */
-#define list_for_each_entry_continue(pos, head, member)                 \
-    for (pos = list_entry(pos->member.next, typeof(*pos), member);      \
-         &pos->member != (head);                                        \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+#define list_for_each_entry_continue(pos, head, member)                   \
+    for ((pos) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
+         &(pos)->member != (head);                                        \
+         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_from - iterate over list of given type from the
@@ -542,8 +542,8 @@  static inline void list_splice_init(struct list_head *list,
  * Iterate over list of given type, continuing from current position.
  */
 #define list_for_each_entry_from(pos, head, member)                     \
-    for (; &pos->member != (head);                                      \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+    for (; &(pos)->member != (head);                                    \
+         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_safe - iterate over list of given type safe
@@ -554,10 +554,10 @@  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_safe(pos, n, head, member)                  \
-    for (pos = list_entry((head)->next, typeof(*pos), member),          \
-         n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+    for ((pos) = list_entry((head)->next, typeof(*(pos)), member),      \
+         (n) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
+         &(pos)->member != (head);                                      \
+         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_continue
@@ -569,11 +569,11 @@  static inline void list_splice_init(struct list_head *list,
  * Iterate over list of given type, continuing after current point,
  * safe against removal of list entry.
  */
-#define list_for_each_entry_safe_continue(pos, n, head, member)         \
-    for (pos = list_entry(pos->member.next, typeof(*pos), member),      \
-         n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+#define list_for_each_entry_safe_continue(pos, n, head, member)           \
+    for ((pos) = list_entry((pos)->member.next, typeof(*(pos)), member),  \
+         (n) = list_entry((pos)->member.next, typeof(*(pos)), member);    \
+         &(pos)->member != (head);                                        \
+         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_from
@@ -586,9 +586,9 @@  static inline void list_splice_init(struct list_head *list,
  * removal of list entry.
  */
 #define list_for_each_entry_safe_from(pos, n, head, member)             \
-    for (n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+    for ((n) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
+         &(pos)->member != (head);                                      \
+         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_reverse
@@ -601,10 +601,10 @@  static inline void list_splice_init(struct list_head *list,
  * of list entry.
  */
 #define list_for_each_entry_safe_reverse(pos, n, head, member)          \
-    for (pos = list_entry((head)->prev, typeof(*pos), member),          \
-         n = list_entry(pos->member.prev, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.prev, typeof(*n), member))
+    for ((pos) = list_entry((head)->prev, typeof(*(pos)), member),      \
+         (n) = list_entry((pos)->member.prev, typeof(*(pos)), member);  \
+         &(pos)->member != (head);                                      \
+         (pos) = (n), (n) = list_entry((n)->member.prev, typeof(*(n)), member))
 
 /**
  * list_for_each_rcu - iterate over an rcu-protected list
@@ -616,14 +616,14 @@  static inline void list_splice_init(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_rcu(pos, head)                            \
-    for (pos = (head)->next;                                    \
+    for ((pos) = (head)->next;                                  \
          rcu_dereference(pos) != (head);                        \
-         pos = pos->next)
+         (pos) = (pos)->next)
 
 #define __list_for_each_rcu(pos, head)          \
-    for (pos = (head)->next;                    \
+    for ((pos) = (head)->next;                  \
          rcu_dereference(pos) != (head);        \
-         pos = pos->next)
+         (pos) = (pos)->next)
 
 /**
  * list_for_each_safe_rcu
@@ -638,9 +638,9 @@  static inline void list_splice_init(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_safe_rcu(pos, n, head)            \
-    for (pos = (head)->next;                            \
-         n = rcu_dereference(pos)->next, pos != (head); \
-         pos = n)
+    for ((pos) = (head)->next;                          \
+         (n) = rcu_dereference(pos)->next, (pos) != (head); \
+         (pos) = (n))
 
 /**
  * list_for_each_entry_rcu - iterate over rcu list of given type
@@ -653,9 +653,9 @@  static inline void list_splice_init(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_entry_rcu(pos, head, member)                      \
-    for (pos = list_entry((head)->next, typeof(*pos), member);          \
+    for ((pos) = list_entry((head)->next, typeof(*(pos)), member);      \
          &rcu_dereference(pos)->member != (head);                       \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_continue_rcu
@@ -977,4 +977,3 @@  static inline void hlist_add_after_rcu(struct hlist_node *prev,
           pos = pos->next)
 
 #endif /* __XEN_LIST_H__ */
-