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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxx>
>> ---
>>  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