Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

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

On 05/11/13 10:40, Jan Beulich wrote:
>>>> On 04.11.13 at 22:30, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> -static char *pointer(char *str, char *end,
>> +static char *pointer(char *str, char *end, const char **fmt_ptr,
>>                       unsigned long val, int field_width, int precision,
>>                       int flags)
>>  {
>> -    if ( field_width == -1 )
>> +    const char *fmt = *fmt_ptr, *s;
>> +
>> +    /*
>> +     * Custom %p suffixes, compatible with Linux.
>> +     * See XEN_ROOT/docs/misc/printk-formats.txt
>> +     */
>> +    switch ( fmt[1] )
>>      {
>> -        field_width = 2 * sizeof(void *);
>> -        flags |= ZEROPAD;
>> +    case 's': /* Symbol name */
>> +    case 'S': /* Symbol name with offset and size */
>> +    {
>> +        unsigned long sym_size, sym_offset;
>> +        char namebuf[KSYM_NAME_LEN+1];
>> +
>> +        /* Advance fmt pointer, as we have consumed 's' or 'S'.  Leave fmt
>> +         * along for consistency. */
> I'm not sure what the second sentence is supposed to tell me, even
> after s/along/alone/. Also - coding style (multi-line comment), no
> matter that other coding style aspects are being done matching the
> rest of the file rather than ./CODING_STYLE.

It was left over from a previous iteration which I thought I had
purged.  It has been now.

>
>> +        (*fmt_ptr)++;
> ++*fmt_ptr allows avoiding the parentheses.
>
>> +
>> +        s = symbols_lookup(val, &sym_size, &sym_offset, namebuf);
>> +
>> +        /* If the symbol is not found, fall back to printing the address */
>> +        if ( !s )
>> +            goto regular_pointer;
> Rather than using "goto" here, I'd suggest using "break" and
> handling the "normal" pointer case outside of the switch.
>
> Also I'm inclined to suggest that plain symbols be printed as
> symbols only when sym_offset is zero (otherwise confusion arises
> as to whether a pointer was an exact match).
>
>> +
>> +        /* Print symbol name */
>> +        str = string(str, end, s, -1, -1, 0);
>> +
>> +        if ( fmt[1] == 'S' )
>> +        {
>> +            /* Print '+0x<offset>/0x<len>' */
>> +            str = string(str, end, "+0x", -1, -1, 0);
>> +            str = number(str, end, sym_offset, 16, -1, -1, 0);
> I'd further suggest to pass SPECIAL here and avoid explicitly
> printing 0x (thus allowing at least zero to be printed as plain
> "0").
>
> Similarly passing SIGN|PLUS here would allow you to drop
> the explicit issuing of "+".
>
>> +            str = string(str, end, "/0x", -1, -1, 0);
>> +            str = number(str, end, sym_size, 16, -1, -1, 0);
> Same here then, even though sym_size should rarely end up
> being zero.
>
> Jan
>

Ok for all other points.

~Andrew