Re: [PATCH for-4.5 1/2] xen/vsprintf: Introduce %*ph extended format specifier for hex buffers

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/09/14 12:32, Jan Beulich wrote:
>>>> On 26.09.14 at 12:10, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/printk-formats.txt
>> +++ b/docs/misc/printk-formats.txt
>> @@ -18,3 +18,9 @@ Symbol/Function pointers:
>>  
>>         %pv     Domain and vCPU ID from a 'struct vcpu *' (printed as
>>                 "d<domid>v<vcpuid>")
>> +
>> +
>> +Raw buffer as hex string:
>> +
>> +       %*ph    Up to 64 characters, printed as "00 01 02 ... ff".  Buffer length
>> +               expected via the field_width paramter. i.e. printk("%*ph", 8, buffer);
> Let's keep this list sorted alphabetically please.

Ok, but then the "Symbol/Function pointers:" paragraph marker should be
dropped.

I am happy with doing either.

>
> Also Linux supports an optional suffix (any of C, D, or N).

None of those looked particularly useful IMO.  They would be trivial to
add should a need arise.

>
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -272,6 +272,31 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>>      /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
>>      switch ( fmt[1] )
>>      {
>> +    case 'h': /* Raw buffer as hex string. */
>> +    {
>> +        /*
>> +         * User expected to provide an explicit count using %*.  Bound between
>> +         * 0 and 64 bytes, defaulting to 0.
>> +         */
>> +        unsigned i, nr_bytes =
>> +            ((field_width < 1) || (field_width > 64)) ? 0 : field_width;
> Producing no output for too small a field width makes sense, but why
> not print 64 bytes if more were requested?

64 is arbitrary (taken from the Linux statement to the same effect). 
Even with an upper bound of 64, the caller should be using something
shorter and putting in newlines.

>
>> +        const uint8_t *hex_buffer = arg;
>> +
>> +        /* Consumed 'h' from the format string. */
>> +        ++*fmt_ptr;
>> +
>> +        for ( i = 0; i < nr_bytes; ++i )
>> +        {
>> +            /* Each byte: 2 chars, 0-padded, base 16, no hex prefix. */
>> +            str = number(str, end, hex_buffer[i], 16, 2, -1, ZEROPAD);
>> +            if ( str < end )
>> +                *str = ' ';
>> +            ++str;
> Could you suppress the trailing blank this produces, just like Linux
> does?

I will see what I can do.

~Andrew