[SLOF] [PATCH slof] libc: Port vsnprintf back from skiboot

[Alexey Kardashevskiy]

On 12/01/2015 09:37 PM, Thomas Huth wrote:
> On 01/12/15 08:14, Alexey Kardashevskiy wrote:
>> Since initial port from slof to skiboot, vsnprintf() has improved in
>> skiboot so let's port the improved version back.
>>
>> Suggested-by: Thomas Huth <thuth at redhat.com>
>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>> ---
>>
>> btw the initial commit of it is a bit different from the current slof version.
>
> Yes, looks like the initial commit in skiboot already contained support
> for uppercase hex number printing which is still lacking in the current
> version of SLOF.
>
>> diff --git a/lib/libc/stdio/vsnprintf.c b/lib/libc/stdio/vsnprintf.c
>> index e78fb3d..96a48b9 100644
>> --- a/lib/libc/stdio/vsnprintf.c
>> +++ b/lib/libc/stdio/vsnprintf.c
>> @@ -10,72 +10,112 @@
>>    *     IBM Corporation - initial implementation
>>    *****************************************************************************/
>>
>> +#include <stdbool.h>
>>   #include "stdio.h"
>>   #include "stdlib.h"
>>   #include "string.h"
>> +#include "ctype.h"
>>
>>   const static unsigned long long convert[] = {
>
> Skiboot uses "static const" instead of "const static" ... could you
> maybe change that, too? ... GCC seems to complain about "const static"
> when using the "-Wextra" compiler flag.

Ah, that's why this change is there. Fixed.

>
>>   	0x0, 0xFF, 0xFFFF, 0xFFFFFF, 0xFFFFFFFF,
>>   	0xFFFFFFFFFFULL, 0xFFFFFFFFFFFFULL, 0xFFFFFFFFFFFFFFULL, 0xFFFFFFFFFFFFFFFFULL
>>   };
>>
>> -
>> -
>>   static int
>> -print_itoa(char **buffer,unsigned long value, unsigned short int base)
>> +print_str_fill(char **buffer, size_t bufsize, char *sizec,
>> +					const char *str, char c)
>>   {
>> -	const char zeichen[] = {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
>> -	static char sign = 0;
>> +	int i, sizei, len;
>> +	char *bstart = *buffer;
>>
>> -	if(base <= 2 || base > 16)
>> -		return 0;
>> -
>> -	if(value < 0) {
>> -		sign = 1;
>> -		value *= -1;
>> -	}
>> -
>> -	if(value < base) {
>> -		if(sign) {
>> -			**buffer = '-';
>> +	sizei = strtoul(sizec, NULL, 10);
>> +	len = strlen(str);
>> +	if (sizei > len) {
>> +		for (i = 0;
>> +			(i < (sizei - len)) && ((*buffer - bstart) < bufsize);
>> +									i++) {
>> +			**buffer = c;
>>   			*buffer += 1;
>> -			sign = 0;
>>   		}
>> -		**buffer = zeichen[value];
>> -		*buffer += 1;
>> -	} else {
>> -		print_itoa(buffer, value / base, base);
>> -		**buffer = zeichen[(value % base)];
>> -		*buffer += 1;
>>   	}
>> +	return 1;
>> +}
>>
>> +static int
>> +print_str(char **buffer, size_t bufsize, const char *str)
>> +{
>> +	char *bstart = *buffer;
>> +	int i;
>> +
>> +	for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < bufsize); i++) {
>
> Hmmm, looks like somebody from the skiboot folks is suffering from
> paranthesitis, too ... ;-)

s/suffering/enjoying/ ;)


>
>> +		**buffer = str[i];
>> +		*buffer += 1;
>> +	}
>>   	return 1;
>>   }
>>
>>
>> +
>
> Maybe remove that additional empty line ... skiboot does not have it either.
>
>>   static unsigned int
>>   print_intlen(unsigned long value, unsigned short int base)
>>   {
>>   	int i = 0;
>>
>> -	while(value > 0) {
>> +	while (value > 0) {
>>   		value /= base;
>>   		i++;
>>   	}
>> -	if(i == 0) i = 1;
>> +	if (i == 0)
>> +		i = 1;
>>   	return i;
>>   }
>
> Just some cosmetic nits ... remaining part looks fine to me, so:
>
> Reviewed-by: Thomas Huth <thuth at redhat.com>

Thanks!



-- 
Alexey