1. Patchew
  2. Xen
  3. [RFC PATCH 0/4] address MISRA C:2012 Rule 15.2
  4. View patch

[RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop

Nicola Vetrini posted 4 patches 2 years, 3 months ago
[RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop
Posted by Nicola Vetrini 2 years, 3 months ago 
The backwards goto in the vsnprintf function can be replaced
with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/vsprintf.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index c49631c0a4d8..603bae44177a 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
     }
 
     for (; *fmt ; ++fmt) {
+        bool repeat = true;
+
         if (*fmt != '%') {
             if (str < end)
                 *str = *fmt;
@@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 
         /* process flags */
         flags = 0;
-    repeat:
-        ++fmt;          /* this also skips first '%' */
-        switch (*fmt) {
-        case '-': flags |= LEFT; goto repeat;
-        case '+': flags |= PLUS; goto repeat;
-        case ' ': flags |= SPACE; goto repeat;
-        case '#': flags |= SPECIAL; goto repeat;
-        case '0': flags |= ZEROPAD; goto repeat;
+        while ( repeat ) {
+            ++fmt;          /* this also skips the first '%' */
+            switch (*fmt) {
+            case '-': flags |= LEFT; break;
+            case '+': flags |= PLUS; break;
+            case ' ': flags |= SPACE; break;
+            case '#': flags |= SPECIAL; break;
+            case '0': flags |= ZEROPAD; break;
+            default: repeat = false; break;
+            }
         }
 
         /* get field width */
-- 
2.34.1
Re: [RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop
Posted by Andrew Cooper 2 years, 3 months ago 
On 07/11/2023 10:33 am, Nicola Vetrini wrote:
> The backwards goto in the vsnprintf function can be replaced
> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/common/vsprintf.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index c49631c0a4d8..603bae44177a 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>      }
>  
>      for (; *fmt ; ++fmt) {
> +        bool repeat = true;
> +
>          if (*fmt != '%') {
>              if (str < end)
>                  *str = *fmt;
> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  
>          /* process flags */
>          flags = 0;
> -    repeat:
> -        ++fmt;          /* this also skips first '%' */
> -        switch (*fmt) {
> -        case '-': flags |= LEFT; goto repeat;
> -        case '+': flags |= PLUS; goto repeat;
> -        case ' ': flags |= SPACE; goto repeat;
> -        case '#': flags |= SPECIAL; goto repeat;
> -        case '0': flags |= ZEROPAD; goto repeat;
> +        while ( repeat ) {
> +            ++fmt;          /* this also skips the first '%' */
> +            switch (*fmt) {
> +            case '-': flags |= LEFT; break;
> +            case '+': flags |= PLUS; break;
> +            case ' ': flags |= SPACE; break;
> +            case '#': flags |= SPECIAL; break;
> +            case '0': flags |= ZEROPAD; break;
> +            default: repeat = false; break;
> +            }

I'm firmly against this change.  It takes a simple and clear piece of
code and replaces it with something harder to follow because you have to
look elsewhere to figure how the variable works.

Labels with names such as repeat/again/retry are clearly forming a
loop(ish).

I see in patch 4 that you exempt again/retry.  That list needs to
include repeat, and this patch wants dropping.

~Andrew
Re: [RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop
Posted by Jan Beulich 2 years, 2 months ago 
On 07.11.2023 12:36, Andrew Cooper wrote:
> On 07/11/2023 10:33 am, Nicola Vetrini wrote:
>> The backwards goto in the vsnprintf function can be replaced
>> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/common/vsprintf.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>> index c49631c0a4d8..603bae44177a 100644
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>      }
>>  
>>      for (; *fmt ; ++fmt) {
>> +        bool repeat = true;
>> +
>>          if (*fmt != '%') {
>>              if (str < end)
>>                  *str = *fmt;
>> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>  
>>          /* process flags */
>>          flags = 0;
>> -    repeat:
>> -        ++fmt;          /* this also skips first '%' */
>> -        switch (*fmt) {
>> -        case '-': flags |= LEFT; goto repeat;
>> -        case '+': flags |= PLUS; goto repeat;
>> -        case ' ': flags |= SPACE; goto repeat;
>> -        case '#': flags |= SPECIAL; goto repeat;
>> -        case '0': flags |= ZEROPAD; goto repeat;
>> +        while ( repeat ) {
>> +            ++fmt;          /* this also skips the first '%' */
>> +            switch (*fmt) {
>> +            case '-': flags |= LEFT; break;
>> +            case '+': flags |= PLUS; break;
>> +            case ' ': flags |= SPACE; break;
>> +            case '#': flags |= SPECIAL; break;
>> +            case '0': flags |= ZEROPAD; break;
>> +            default: repeat = false; break;
>> +            }
> 
> I'm firmly against this change.  It takes a simple and clear piece of
> code and replaces it with something harder to follow because you have to
> look elsewhere to figure how the variable works.

While I don't really like that change either, I also don't like uses of
goto (at some point we said using it for error handling is okay, but
the case here is clearly not in that category). So at least for
consideration, how about getting away without a new variable:

        for ( ; ; )
        {
            ++fmt;          /* this also skips the first '%' */
            switch ( *fmt )
            {
            case '-': flags |= LEFT; continue;
            case '+': flags |= PLUS; continue;
            case ' ': flags |= SPACE; continue;
            case '#': flags |= SPECIAL; continue;
            case '0': flags |= ZEROPAD; continue;
            }
            break;
        }

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.