From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Mon, 19 Jan 2009 20:49:16 -0500 Subject: [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf() In-Reply-To: <4973EAC1.1070204@cn.fujitsu.com> References: <496C5A37.2030809@cn.fujitsu.com> <20090115190829.GF32620@Krystal> <4973EAC1.1070204@cn.fujitsu.com> Message-ID: <20090120014916.GC9224@Krystal> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote: > Mathieu Desnoyers wrote: > > * Lai Jiangshan (laijs at cn.fujitsu.com) wrote: > >> introduce ltt_serialize_printf() for format binary data which is in > >> ltt-relay buffer to human readable text string. > >> > >> Signed-off-by: Lai Jiangshan > >> --- > >> b/ltt/ltt-serialize.c | 108 ++++++++++++++++++++++++++++++++++++++++++++- > >> include/linux/ltt-tracer.h | 3 + > >> 2 files changed, 109 insertions(+), 2 deletions(-) > >> diff --git a/include/linux/ltt-tracer.h b/include/linux/ltt-tracer.h > >> index ece6819..b49cac2 100644 > >> --- a/include/linux/ltt-tracer.h > >> +++ b/include/linux/ltt-tracer.h > >> @@ -118,6 +119,9 @@ extern void ltt_vtrace(const struct marker *mdata, void *probe_data, > >> extern void ltt_trace(const struct marker *mdata, void *probe_data, > >> void *call_data, const char *fmt, ...); > >> > >> +size_t ltt_serialize_printf(struct rchan_buf *buf, size_t buf_offset, > >> + u32 *msg_size, char *output, size_t outlen, const char *fmt); > >> + > >> /* > >> * Unique ID assigned to each registered probe. > >> */ > >> diff --git a/ltt/ltt-serialize.c b/ltt/ltt-serialize.c > >> index 5c34899..deb9106 100644 > >> --- a/ltt/ltt-serialize.c > >> +++ b/ltt/ltt-serialize.c > >> @@ -227,7 +227,7 @@ parse_end: > >> * %n not supported. > >> */ > >> static inline const char *parse_c_type(const char *fmt, > >> - char *c_size, enum ltt_type *c_type) > >> + char *c_size, enum ltt_type *c_type, char *outfmt) > >> { > >> int qualifier; /* 'h', 'l', or 'L' for integer fields */ > >> /* 'z' support added 23/7/1999 S.H. */ > >> @@ -259,6 +259,13 @@ repeat: > >> } > >> } > >> > >> + if (outfmt) { > >> + if (qualifier != -1) > >> + *outfmt++ = (char)qualifier; > >> + *outfmt++ = *fmt; > >> + *outfmt = 0; > >> + } > >> + > >> switch (*fmt) { > >> case 'c': > >> *c_type = LTT_TYPE_UNSIGNED_INT; > >> @@ -502,7 +509,7 @@ notrace size_t ltt_serialize_data(struct rchan_buf *buf, size_t buf_offset, > >> ++fmt; /* skip first '%' */ > >> if (*fmt == '%') /* Escaped %% */ > >> break; > >> - fmt = parse_c_type(fmt, &c_size, &c_type); > >> + fmt = parse_c_type(fmt, &c_size, &c_type, NULL); > >> /* > >> * Output c types if no trace types has been > >> * specified. > >> @@ -531,6 +538,103 @@ > >> return value; > >> } > >> > >> +static inline int serialize_printf_data(struct rchan_buf *buf, size_t *ppos, > >> + char trace_size, enum ltt_type trace_type, > >> + char c_size, enum ltt_type c_type, > >> + char *output, size_t outlen, const char *outfmt) > >> +{ > >> + u64 value; > >> + outlen = (ssize_t)outlen < 0 ? 0 : outlen; > >> + > >> + if (trace_type == LTT_TYPE_STRING) { > >> + size_t len = ltt_relay_read_cstr(buf, *ppos, output, outlen); > >> + *ppos += len + 1; > >> + return len; > >> + } > >> + > >> + value = serialize_read_base_type(buf, ppos, trace_size, trace_type); > >> + > >> + if (c_size == 8) > >> + return snprintf(output, outlen, outfmt, value); > >> + else > >> + return snprintf(output, outlen, outfmt, (int)value); > > > > Given value is u64, I'd use unsigned int cast. Should not make much > > difference in the end because there is no sign to extend, but I prefer > > to play safe. > > There are the same. > > > > >> +} > >> + > > > > kerneldocs header would be useful here... what is "output" exactly ? I > > can figure it out, but it took me a few seconds to be sure. :) > > > > Does outlen include space for the final \0 ? Or maybe there is no final > > \0 at the end of output ... this should be specified. > > > >> +size_t ltt_serialize_printf(struct rchan_buf *buf, size_t buf_offset, > >> + u32 *msg_size, char *output, size_t outlen, const char *fmt) > >> +{ > >> + char trace_size = 0, c_size = 0; /* > >> + * 0 (unset), 1, 2, 4, 8 bytes. > >> + */ > >> + enum ltt_type trace_type = LTT_TYPE_NONE, c_type = LTT_TYPE_NONE; > >> + unsigned long attributes = 0; > >> + char outfmt[4] = "%"; > >> + size_t outpos = 0, len; > > > > I find the line above surprising. can we declare len on a separate line? > > Or we should concatenate the next msgpos = buf_offset line... > > How surprise comes from? how about I rename len to outedlen? > len is used for output buffer, I think "size_t outpos = 0, len;" is good. > > msgpos is for relay buffer. I like the "len" variable name, this is not the problem. The only thing I did not like is having a variable declaration and assignment, followed by a comma "," and then another declaration without assignment. It's just for readability. I would prefer size_t outpos = 0; size_t len; to size_t outpos = 0, len; (it's just that it's easy for the mind to get confused when it sees ", len", and I try to confuse LKML reviewers as little as possible) :-) Mathieu > > > >> + size_t msgpos = buf_offset; > >> + outlen = (ssize_t)outlen < 0 ? 0 : outlen; > > > > would be clearer with > > outlen = min_t(ssize_t, 0, outlen); > > > >> + > >> + for (; *fmt ; ++fmt) { > >> + switch (*fmt) { > >> + case '#': > >> + /* tracetypes (#) */ > >> + ++fmt; /* skip first '#' */ > >> + if (*fmt == '#') { /* Escaped ## */ > >> + if (outpos < outlen) > >> + output[outpos] = '#'; > >> + outpos++; > >> + break; > >> + } > >> + attributes = 0; > >> + fmt = parse_trace_type(fmt, &trace_size, &trace_type, > >> + &attributes); > >> + break; > >> + case '%': > >> + /* c types (%) */ > >> + ++fmt; /* skip first '%' */ > >> + if (*fmt == '%') { /* Escaped %% */ > >> + if (outpos < outlen) > >> + output[outpos] = '%'; > >> + outpos++; > >> + break; > >> + } > >> + fmt = parse_c_type(fmt, &c_size, &c_type, outfmt + 1); > >> + /* > >> + * Output c types if no trace types has been > >> + * specified. > >> + */ > >> + if (!trace_size) > >> + trace_size = c_size; > >> + if (trace_type == LTT_TYPE_NONE) > >> + trace_type = c_type; > >> + if (c_type == LTT_TYPE_STRING) > >> + trace_type = LTT_TYPE_STRING; > >> + > >> + /* perform trace printf */ > >> + len = serialize_printf_data(buf, &msgpos, trace_size, > >> + trace_type, c_size, c_type, > >> + output + outpos, outlen - outpos, > >> + outfmt); > >> + outpos += len; > >> + trace_size = 0; > >> + c_size = 0; > >> + trace_type = LTT_TYPE_NONE; > >> + c_size = LTT_TYPE_NONE; > >> + attributes = 0; > >> + break; > >> + default: > >> + if (outpos < outlen) > >> + output[outpos] = *fmt; > >> + outpos++; > >> + break; > >> + } > >> + } > > > > Mathieu > > > >> + if (msg_size) > >> + *msg_size = msgpos - buf_offset; > >> + > >> + return outpos; > >> +} > >> +EXPORT_SYMBOL_GPL(ltt_serialize_printf); > >> + > >> /* > >> * Calculate data size > >> * Assume that the padding for alignment starts at a sizeof(void *) address. > >> > >> > >> > >> > >> _______________________________________________ > >> ltt-dev mailing list > >> ltt-dev at lists.casi.polymtl.ca > >> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > >> > > > > > > _______________________________________________ > ltt-dev mailing list > ltt-dev at lists.casi.polymtl.ca > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68