From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for-4.5 1/2] xen/vsprintf: Introduce %*ph extended format specifier for hex buffers Date: Fri, 26 Sep 2014 13:16:06 +0100 Message-ID: <54255906.8010405@citrix.com> References: <1411726207-2689-1-git-send-email-andrew.cooper3@citrix.com> <1411726207-2689-2-git-send-email-andrew.cooper3@citrix.com> <54256AF90200007800039974@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54256AF90200007800039974@mail.emea.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: Tim Deegan , Keir Fraser , Xen-devel List-Id: xen-devel@lists.xenproject.org On 26/09/14 12:32, Jan Beulich wrote: >>>> On 26.09.14 at 12:10, 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 >> "dv") >> + >> + >> +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