[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()

[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()

[Lai Jiangshan]

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.

[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()

[Mathieu Desnoyers]

* 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

[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()

[Lai Jiangshan]

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
>>
> 

[ltt-dev] [PATCH 06/13] introduce ltt_serialize_printf()

[Mathieu Desnoyers]

* 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