* [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()
@ 2009-01-13 9:09 Lai Jiangshan
2009-01-15 19:08 ` Mathieu Desnoyers
0 siblings, 1 reply; 4+ messages in thread
From: Lai Jiangshan @ 2009-01-13 9:09 UTC (permalink / raw)
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);
+}
+
+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;
+ size_t msgpos = buf_offset;
+ outlen = (ssize_t)outlen < 0 ? 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;
+ }
+ }
+ 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.
^ permalink raw reply [flat|nested] 4+ messages in thread* [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()
2009-01-13 9:09 [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf() Lai Jiangshan
@ 2009-01-15 19:08 ` Mathieu Desnoyers
2009-01-19 2:51 ` Lai Jiangshan
0 siblings, 1 reply; 4+ messages in thread
From: Mathieu Desnoyers @ 2009-01-15 19:08 UTC (permalink / raw)
* 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.
> +}
> +
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...
> + 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
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
^ permalink raw reply [flat|nested] 4+ messages in thread* [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()
2009-01-15 19:08 ` Mathieu Desnoyers
@ 2009-01-19 2:51 ` Lai Jiangshan
2009-01-20 1:49 ` Mathieu Desnoyers
0 siblings, 1 reply; 4+ messages in thread
From: Lai Jiangshan @ 2009-01-19 2:51 UTC (permalink / raw)
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.
>
>> + 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
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()
2009-01-19 2:51 ` Lai Jiangshan
@ 2009-01-20 1:49 ` Mathieu Desnoyers
0 siblings, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2009-01-20 1:49 UTC (permalink / raw)
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-01-20 1:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-13 9:09 [ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf() Lai Jiangshan
2009-01-15 19:08 ` Mathieu Desnoyers
2009-01-19 2:51 ` Lai Jiangshan
2009-01-20 1:49 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox