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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.11.13 at 22:30, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.

> +        (*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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel