1. Patchew
  2. Xen
  3. View series

[XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1

Nicola Vetrini posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/9a27f6cbe552a00274f7ad32eec63f0e80e7644f.1689176790.git.nicola.vetrini@bugseng.com
xen/common/xmalloc_tlsf.c | 13 ++++++++++---
xen/include/xen/atomic.h  |  2 +-
2 files changed, 11 insertions(+), 4 deletions(-)
[XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1
Posted by Nicola Vetrini 1 year, 2 months ago 
In the file 'xen/common/xmalloc_tlsf.c' is not clear how
the commented-out code should interact with the previous statement.
To resolve the MISRA violation generated by the nested comment
a #if .. #endif block with an explanatory comment substitutes
the earlier construct.

In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
since the code sample is already explained by the preceding comment.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Following the suggestion of this message
https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/
an explanatory comment has been added.
---
 xen/common/xmalloc_tlsf.c | 13 ++++++++++---
 xen/include/xen/atomic.h  |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index c21bf71e88..56c3849414 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
         *r = *r + t;
         *fl = flsl(*r) - 1;
         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
-        *fl -= FLI_OFFSET;
-        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
-         *fl = *sl = 0;
+        /* 
+         * It's unclear what was the purpose of the commented-out code that now
+         * is in the #else branch. The current form is motivated by the correction
+         * of a violation MISRA:C 2012 Rule 3.1
          */
+#if 1
+        *fl -= FLI_OFFSET;
+#else
+        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
+          *fl = *sl = 0;
+#endif
         *r &= ~t;
     }
 }
diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
index 529213ebbb..fa750a18ae 100644
--- a/xen/include/xen/atomic.h
+++ b/xen/include/xen/atomic.h
@@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i);
  *      int old = atomic_read(&v);
  *      int new = old + 1;
  *      if ( likely(old == atomic_cmpxchg(&v, old, new)) )
- *          break; // success!
+ *          break;
  *  }
  */
 static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
-- 
2.34.1
Re: [XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1
Posted by Stefano Stabellini 1 year, 2 months ago 
On Wed, 12 Jul 2023, Nicola Vetrini wrote:
> In the file 'xen/common/xmalloc_tlsf.c' is not clear how
> the commented-out code should interact with the previous statement.
> To resolve the MISRA violation generated by the nested comment
> a #if .. #endif block with an explanatory comment substitutes
> the earlier construct.
> 
> In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
> since the code sample is already explained by the preceding comment.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Following the suggestion of this message
> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/
> an explanatory comment has been added.
> ---
>  xen/common/xmalloc_tlsf.c | 13 ++++++++++---
>  xen/include/xen/atomic.h  |  2 +-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index c21bf71e88..56c3849414 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>          *r = *r + t;
>          *fl = flsl(*r) - 1;
>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
> -        *fl -= FLI_OFFSET;
> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> -         *fl = *sl = 0;
> +        /* 
> +         * It's unclear what was the purpose of the commented-out code that now
> +         * is in the #else branch. The current form is motivated by the correction
> +         * of a violation MISRA:C 2012 Rule 3.1
>           */
> +#if 1
> +        *fl -= FLI_OFFSET;
> +#else
> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> +          *fl = *sl = 0;
> +#endif
>          *r &= ~t;
>      }
>  }
> diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
> index 529213ebbb..fa750a18ae 100644
> --- a/xen/include/xen/atomic.h
> +++ b/xen/include/xen/atomic.h
> @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i);
>   *      int old = atomic_read(&v);
>   *      int new = old + 1;
>   *      if ( likely(old == atomic_cmpxchg(&v, old, new)) )
> - *          break; // success!
> + *          break;
>   *  }
>   */
>  static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
> -- 
> 2.34.1
>
Re: [XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1
Posted by Luca Fancellu 1 year, 2 months ago 

> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> In the file 'xen/common/xmalloc_tlsf.c' is not clear how
> the commented-out code should interact with the previous statement.
> To resolve the MISRA violation generated by the nested comment
> a #if .. #endif block with an explanatory comment substitutes
> the earlier construct.
> 
> In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
> since the code sample is already explained by the preceding comment.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Following the suggestion of this message
> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/
> an explanatory comment has been added.
> ---
> xen/common/xmalloc_tlsf.c | 13 ++++++++++---
> xen/include/xen/atomic.h  |  2 +-
> 2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index c21bf71e88..56c3849414 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>         *r = *r + t;
>         *fl = flsl(*r) - 1;
>         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
> -        *fl -= FLI_OFFSET;
> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> -         *fl = *sl = 0;
> +        /* 
> +         * It's unclear what was the purpose of the commented-out code that now
> +         * is in the #else branch. The current form is motivated by the correction
> +         * of a violation MISRA:C 2012 Rule 3.1
>          */
> +#if 1
> +        *fl -= FLI_OFFSET;
> +#else
> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!

In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed?
Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you
are just moving code...
So maybe the maintainer can tell what is the best thing to do here.

> +          *fl = *sl = 0;
> +#endif
>         *r &= ~t;
>     }
> }
> diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h
> index 529213ebbb..fa750a18ae 100644
> --- a/xen/include/xen/atomic.h
> +++ b/xen/include/xen/atomic.h
> @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i);
>  *      int old = atomic_read(&v);
>  *      int new = old + 1;
>  *      if ( likely(old == atomic_cmpxchg(&v, old, new)) )
> - *          break; // success!
> + *          break;
>  *  }
>  */
> static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
> -- 
> 2.34.1
> 
> 

Anyway apart from that, the patch looks ok to me.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Re: [XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1
Posted by Nicola Vetrini 1 year, 2 months ago 

On 12/07/23 18:02, Luca Fancellu wrote:
> 
> 
>> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>
>> In the file 'xen/common/xmalloc_tlsf.c' is not clear how
>> the commented-out code should interact with the previous statement.
>> To resolve the MISRA violation generated by the nested comment
>> a #if .. #endif block with an explanatory comment substitutes
>> the earlier construct.
>>
>> In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
>> since the code sample is already explained by the preceding comment.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Following the suggestion of this message
>> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/
>> an explanatory comment has been added.
>> ---
>> xen/common/xmalloc_tlsf.c | 13 ++++++++++---
>> xen/include/xen/atomic.h  |  2 +-
>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>> index c21bf71e88..56c3849414 100644
>> --- a/xen/common/xmalloc_tlsf.c
>> +++ b/xen/common/xmalloc_tlsf.c
>> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>>          *r = *r + t;
>>          *fl = flsl(*r) - 1;
>>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>> -        *fl -= FLI_OFFSET;
>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> -         *fl = *sl = 0;
>> +        /*
>> +         * It's unclear what was the purpose of the commented-out code that now
>> +         * is in the #else branch. The current form is motivated by the correction
>> +         * of a violation MISRA:C 2012 Rule 3.1
>>           */
>> +#if 1
>> +        *fl -= FLI_OFFSET;
>> +#else
>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> 
> In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed?
> Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you
> are just moving code...
> So maybe the maintainer can tell what is the best thing to do here.

You didn't miss any further comment: my suggestion was related to the 
explanatory comment, not the nested comment itself. If a better wording 
can be found for the former, no problem. As for the codestyle point: it 
does not change anything doing
"// FL will be always >0!" -> "/* FL will be always >0!  */
w.r.t. Rule 3.1 (both would have been nested comments).

Regards,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH] xen: fix violations of MISRA C:2012 Rule 3.1
Posted by Luca Fancellu 1 year, 2 months ago 

> On 12 Jul 2023, at 17:17, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> 
> 
> On 12/07/23 18:02, Luca Fancellu wrote:
>>> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>> 
>>> In the file 'xen/common/xmalloc_tlsf.c' is not clear how
>>> the commented-out code should interact with the previous statement.
>>> To resolve the MISRA violation generated by the nested comment
>>> a #if .. #endif block with an explanatory comment substitutes
>>> the earlier construct.
>>> 
>>> In the file 'xen/include/xen/atomic.h' the nested comment has been removed,
>>> since the code sample is already explained by the preceding comment.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> Following the suggestion of this message
>>> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/
>>> an explanatory comment has been added.
>>> ---
>>> xen/common/xmalloc_tlsf.c | 13 ++++++++++---
>>> xen/include/xen/atomic.h  |  2 +-
>>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
>>> index c21bf71e88..56c3849414 100644
>>> --- a/xen/common/xmalloc_tlsf.c
>>> +++ b/xen/common/xmalloc_tlsf.c
>>> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
>>>         *r = *r + t;
>>>         *fl = flsl(*r) - 1;
>>>         *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>> -        *fl -= FLI_OFFSET;
>>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>>> -         *fl = *sl = 0;
>>> +        /*
>>> +         * It's unclear what was the purpose of the commented-out code that now
>>> +         * is in the #else branch. The current form is motivated by the correction
>>> +         * of a violation MISRA:C 2012 Rule 3.1
>>>          */
>>> +#if 1
>>> +        *fl -= FLI_OFFSET;
>>> +#else
>>> +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>> In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed?
>> Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you
>> are just moving code...
>> So maybe the maintainer can tell what is the best thing to do here.
> 
> You didn't miss any further comment: my suggestion was related to the explanatory comment, not the nested comment itself. If a better wording can be found for the former, no problem. As for the codestyle point: it does not change anything doing
> "// FL will be always >0!" -> "/* FL will be always >0!  */
> w.r.t. Rule 3.1 (both would have been nested comments).

Yes, I agree it does not change anything, now that I read better the message, it is from Jan suggesting this:

#if 1
    *fl -= FLI_OFFSET;
#else
    if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */
        *fl = *sl = 0;
#endif

So using /* FL will be always >0! */ instead of copying // FL will be always >0!

Anyway, I think it can be addressed on commit, whatever form the maintainer prefers.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.