From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support Date: Tue, 5 Nov 2013 10:57:20 +0000 Message-ID: <5278CF10.5020706@citrix.com> References: <1383600624-6345-1-git-send-email-andrew.cooper3@citrix.com> <1383600624-6345-4-git-send-email-andrew.cooper3@citrix.com> <5278D93F02000078000FF731@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VdeKO-0000VD-Eh for xen-devel@lists.xenproject.org; Tue, 05 Nov 2013 10:57:44 +0000 In-Reply-To: <5278D93F02000078000FF731@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 05/11/13 10:40, Jan Beulich wrote: >>>> On 04.11.13 at 22:30, Andrew Cooper 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/0x' */ >> + 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