[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()
Mathieu Desnoyers
compudj at krystal.dyndns.org
Mon Jan 19 20:49:16 EST 2009
* 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 <laijs at cn.fujitsu.com>
> >> ---
> >> 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
More information about the lttng-dev
mailing list