Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] trace-cmd: Add support for more printk format specifiers
@ 2020-05-12 15:51 Tzvetomir Stoyanov (VMware)
  2020-05-13  1:14 ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2020-05-12 15:51 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, johannes

The printk format specifiers used in event's print format files extend
the standard printf formats. There are a lot of new options related to
printing pointers and kernel specific structures. Currently trace-cmd
does not support many of them.

Support for these new printk specifiers is added to the pretty_print()
function:
 - UUID/GUID address: %pU[bBlL]
 - Raw buffer as a hex string: %*ph[CDN]

These are improved:
 - MAC address: %pMF, %pM and %pmR
 - IPv4 adderss: %p[Ii]4[hnbl]

Function pretty_print() is refactored. The logic for printing pointers
%p[...] is moved to its own function.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207605
Reported-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
v2 changes: Addressed Steven's comments 
   - Small code style changes in print_mac_arg()
   - Check for file byte order instead of host byte order in
     parse_ip4_print_args()

 lib/traceevent/event-parse.c | 362 ++++++++++++++++++++++++++++-------
 1 file changed, 288 insertions(+), 74 deletions(-)

diff --git a/lib/traceevent/event-parse.c b/lib/traceevent/event-parse.c
index 064c100d..47a30738 100644
--- a/lib/traceevent/event-parse.c
+++ b/lib/traceevent/event-parse.c
@@ -4510,43 +4510,92 @@ get_bprint_format(void *data, int size __maybe_unused,
 	return format;
 }
 
-static void print_mac_arg(struct trace_seq *s, int mac, void *data, int size,
-			  struct tep_event *event, struct tep_print_arg *arg)
+static int print_mac_arg(struct trace_seq *s, const char *format,
+			 void *data, int size, struct tep_event *event,
+			 struct tep_print_arg *arg)
 {
-	unsigned char *buf;
 	const char *fmt = "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x";
+	bool reverse = false;
+	unsigned char *buf;
+	int ret = 0;
 
 	if (arg->type == TEP_PRINT_FUNC) {
 		process_defined_func(s, data, size, event, arg);
-		return;
+		return 0;
 	}
 
 	if (arg->type != TEP_PRINT_FIELD) {
 		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d",
 				 arg->type);
-		return;
+		return 0;
 	}
 
-	if (mac == 'm')
+	if (format[0] == 'm') {
 		fmt = "%.2x%.2x%.2x%.2x%.2x%.2x";
+	} else if (format[0] == 'M' && format[1] == 'F') {
+		fmt = "%.2x-%.2x-%.2x-%.2x-%.2x-%.2x";
+		ret++;
+	}
+	if (format[1] == 'R') {
+		reverse = true;
+		ret++;
+	}
+
 	if (!arg->field.field) {
 		arg->field.field =
 			tep_find_any_field(event, arg->field.name);
 		if (!arg->field.field) {
 			do_warning_event(event, "%s: field %s not found",
 					 __func__, arg->field.name);
-			return;
+			return ret;
 		}
 	}
 	if (arg->field.field->size != 6) {
 		trace_seq_printf(s, "INVALIDMAC");
-		return;
+		return ret;
 	}
+
 	buf = data + arg->field.field->offset;
-	trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+	if (reverse)
+		trace_seq_printf(s, fmt, buf[5], buf[4], buf[3], buf[2], buf[1], buf[0]);
+	else
+		trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+
+	return ret;
 }
 
-static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
+static int parse_ip4_print_args(struct tep_handle *tep,
+				const char *ptr, bool *reverse)
+{
+	int ret = 0;
+
+	*reverse = false;
+
+	/* hnbl */
+	switch (*ptr) {
+	case 'h':
+		if (tep->file_bigendian)
+			*reverse = false;
+		else
+			*reverse = true;
+		ret++;
+	break;
+	case 'l':
+		*reverse = true;
+		ret++;
+	break;
+	case 'n':
+	case 'b':
+		ret++;
+		/* fall through */
+	default:
+		*reverse = false;
+		break;
+	}
+
+	return ret;
+}
+static void print_ip4_addr(struct trace_seq *s, char i, bool reverse, unsigned char *buf)
 {
 	const char *fmt;
 
@@ -4555,7 +4604,11 @@ static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
 	else
 		fmt = "%d.%d.%d.%d";
 
-	trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
+	if (reverse)
+		trace_seq_printf(s, fmt, buf[3], buf[2], buf[1], buf[0]);
+	else
+		trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
+
 }
 
 static inline bool ipv6_addr_v4mapped(const struct in6_addr *a)
@@ -4638,7 +4691,7 @@ static void print_ip6c_addr(struct trace_seq *s, unsigned char *addr)
 	if (useIPv4) {
 		if (needcolon)
 			trace_seq_printf(s, ":");
-		print_ip4_addr(s, 'I', &in6.s6_addr[12]);
+		print_ip4_addr(s, 'I', false, &in6.s6_addr[12]);
 	}
 
 	return;
@@ -4667,16 +4720,20 @@ static int print_ipv4_arg(struct trace_seq *s, const char *ptr, char i,
 			  void *data, int size, struct tep_event *event,
 			  struct tep_print_arg *arg)
 {
+	bool reverse = false;
 	unsigned char *buf;
+	int ret;
+
+	ret = parse_ip4_print_args(event->tep, ptr, &reverse);
 
 	if (arg->type == TEP_PRINT_FUNC) {
 		process_defined_func(s, data, size, event, arg);
-		return 0;
+		return ret;
 	}
 
 	if (arg->type != TEP_PRINT_FIELD) {
 		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
-		return 0;
+		return ret;
 	}
 
 	if (!arg->field.field) {
@@ -4685,7 +4742,7 @@ static int print_ipv4_arg(struct trace_seq *s, const char *ptr, char i,
 		if (!arg->field.field) {
 			do_warning("%s: field %s not found",
 				   __func__, arg->field.name);
-			return 0;
+			return ret;
 		}
 	}
 
@@ -4693,11 +4750,12 @@ static int print_ipv4_arg(struct trace_seq *s, const char *ptr, char i,
 
 	if (arg->field.field->size != 4) {
 		trace_seq_printf(s, "INVALIDIPv4");
-		return 0;
+		return ret;
 	}
-	print_ip4_addr(s, i, buf);
 
-	return 0;
+	print_ip4_addr(s, i, reverse, buf);
+	return ret;
+
 }
 
 static int print_ipv6_arg(struct trace_seq *s, const char *ptr, char i,
@@ -4757,7 +4815,9 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 	char have_c = 0, have_p = 0;
 	unsigned char *buf;
 	struct sockaddr_storage *sa;
+	bool reverse = false;
 	int rc = 0;
+	int ret;
 
 	/* pISpc */
 	if (i == 'I') {
@@ -4772,6 +4832,9 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 			rc++;
 		}
 	}
+	ret = parse_ip4_print_args(event->tep, ptr, &reverse);
+	ptr += ret;
+	rc += ret;
 
 	if (arg->type == TEP_PRINT_FUNC) {
 		process_defined_func(s, data, size, event, arg);
@@ -4803,7 +4866,7 @@ static int print_ipsa_arg(struct trace_seq *s, const char *ptr, char i,
 			return rc;
 		}
 
-		print_ip4_addr(s, i, (unsigned char *) &sa4->sin_addr);
+		print_ip4_addr(s, i, reverse, (unsigned char *) &sa4->sin_addr);
 		if (have_p)
 			trace_seq_printf(s, ":%d", ntohs(sa4->sin_port));
 
@@ -4837,25 +4900,20 @@ static int print_ip_arg(struct trace_seq *s, const char *ptr,
 			struct tep_print_arg *arg)
 {
 	char i = *ptr;  /* 'i' or 'I' */
-	char ver;
-	int rc = 0;
+	int rc = 1;
 
+	/* IP version */
 	ptr++;
-	rc++;
 
-	ver = *ptr;
-	ptr++;
-	rc++;
-
-	switch (ver) {
+	switch (*ptr) {
 	case '4':
-		rc += print_ipv4_arg(s, ptr, i, data, size, event, arg);
+		rc += print_ipv4_arg(s, ptr + 1, i, data, size, event, arg);
 		break;
 	case '6':
-		rc += print_ipv6_arg(s, ptr, i, data, size, event, arg);
+		rc += print_ipv6_arg(s, ptr + 1, i, data, size, event, arg);
 		break;
 	case 'S':
-		rc += print_ipsa_arg(s, ptr, i, data, size, event, arg);
+		rc += print_ipsa_arg(s, ptr + 1, i, data, size, event, arg);
 		break;
 	default:
 		return 0;
@@ -4864,6 +4922,132 @@ static int print_ip_arg(struct trace_seq *s, const char *ptr,
 	return rc;
 }
 
+int  guid_index[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15};
+int  uuid_index[16] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
+static int print_uuid_arg(struct trace_seq *s, const char *ptr,
+			void *data, int size, struct tep_event *event,
+			struct tep_print_arg *arg)
+{
+	int *index = uuid_index;
+	char *format = "%02x";
+	int ret = 0;
+	char *buf;
+	int i;
+
+	switch (*(ptr + 1)) {
+	case 'L':
+		format = "%02X";
+		/* fall through */
+	case 'l':
+		index = guid_index;
+		ret++;
+		break;
+	case 'B':
+		format = "%02X";
+		/* fall through */
+	case 'b':
+		ret++;
+		break;
+	}
+
+	if (arg->type == TEP_PRINT_FUNC) {
+		process_defined_func(s, data, size, event, arg);
+		return ret;
+	}
+
+	if (arg->type != TEP_PRINT_FIELD) {
+		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
+		return ret;
+	}
+
+	if (!arg->field.field) {
+		arg->field.field =
+			tep_find_any_field(event, arg->field.name);
+		if (!arg->field.field) {
+			do_warning("%s: field %s not found",
+				   __func__, arg->field.name);
+			return ret;
+		}
+	}
+
+	if (arg->field.field->size != 16) {
+		trace_seq_printf(s, "INVALIDUUID");
+		return ret;
+	}
+
+	buf = data + arg->field.field->offset;
+
+	for (i = 0; i < 16; i++) {
+		trace_seq_printf(s, format, buf[index[i]] & 0xff);
+		switch (i) {
+		case 3:
+		case 5:
+		case 7:
+		case 9:
+			trace_seq_printf(s, "-");
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int print_raw_buff_arg(struct trace_seq *s, const char *ptr,
+			      void *data, int size, struct tep_event *event,
+			      struct tep_print_arg *arg, int print_len)
+{
+	int plen = print_len;
+	char *delim = " ";
+	int ret = 0;
+	char *buf;
+	int i;
+	unsigned long offset;
+	int arr_len;
+
+	switch (*(ptr + 1)) {
+	case 'C':
+		delim = ":";
+		ret++;
+		break;
+	case 'D':
+		delim = "-";
+		ret++;
+		break;
+	case 'N':
+		delim = "";
+		ret++;
+		break;
+	}
+
+	if (arg->type == TEP_PRINT_FUNC) {
+		process_defined_func(s, data, size, event, arg);
+		return ret;
+	}
+
+	if (arg->type != TEP_PRINT_DYNAMIC_ARRAY) {
+		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
+		return ret;
+	}
+
+	offset = tep_read_number(event->tep,
+				 data + arg->dynarray.field->offset,
+				 arg->dynarray.field->size);
+	arr_len = (unsigned long long)(offset >> 16);
+	buf = data + (offset & 0xffff);
+
+	if (arr_len < plen)
+		plen = arr_len;
+
+	if (plen < 1)
+		return ret;
+
+	trace_seq_printf(s, "%02x", buf[0] & 0xff);
+	for (i = 1; i < plen; i++)
+		trace_seq_printf(s, "%s%02x", delim, buf[i] & 0xff);
+
+	return ret;
+}
+
 static int is_printable_array(char *p, unsigned int len)
 {
 	unsigned int i;
@@ -4952,6 +5136,71 @@ void tep_print_fields(struct trace_seq *s, void *data,
 	}
 }
 
+static int print_function(struct trace_seq *s, const char *format,
+			  void *data, int size, struct tep_event *event,
+			  struct tep_print_arg *arg)
+{
+	struct func_map *func;
+	unsigned long long val;
+
+	val = eval_num_arg(data, size, event, arg);
+	func = find_func(event->tep, val);
+	if (func) {
+		trace_seq_puts(s, func->func);
+		if (*format == 'F' || *format == 'S')
+			trace_seq_printf(s, "+0x%llx", val - func->addr);
+	} else {
+		if (event->tep->long_size == 4)
+			trace_seq_printf(s, "0x%lx", (long)val);
+		else
+			trace_seq_printf(s, "0x%llx", (long long)val);
+	}
+
+	return 0;
+}
+
+static int print_pointer(struct trace_seq *s, const char *format, int plen,
+			 void *data, int size,
+			 struct tep_event *event, struct tep_print_arg *arg)
+{
+	unsigned long long val;
+	int ret = 1;
+
+	switch (*format) {
+	case 'F':
+	case 'f':
+	case 'S':
+	case 's':
+		ret += print_function(s, format, data, size, event, arg);
+		break;
+	case 'M':
+	case 'm':
+		ret += print_mac_arg(s, format, data, size, event, arg);
+		break;
+	case 'I':
+	case 'i':
+		ret += print_ip_arg(s, format, data, size, event, arg);
+		break;
+	case 'U':
+		ret += print_uuid_arg(s, format, data, size, event, arg);
+		break;
+	case 'h':
+		ret += print_raw_buff_arg(s, format, data, size, event, arg, plen);
+		break;
+	default:
+		ret = 0;
+		val = eval_num_arg(data, size, event, arg);
+		if (event->tep->long_size == 4)
+			trace_seq_printf(s, "0x%lx", (long)val);
+		else
+			trace_seq_printf(s, "0x%llx", (long long)val);
+		break;
+	}
+
+	return ret;
+
+}
+
 static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_event *event)
 {
 	struct tep_handle *tep = event->tep;
@@ -4960,16 +5209,15 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
 	struct tep_print_arg *args = NULL;
 	const char *ptr = print_fmt->format;
 	unsigned long long val;
-	struct func_map *func;
 	const char *saveptr;
 	struct trace_seq p;
 	char *bprint_fmt = NULL;
 	char format[32];
-	int show_func;
 	int len_as_arg;
 	int len_arg = 0;
 	int len;
 	int ls;
+	int ret;
 
 	if (event->flags & TEP_EVENT_FL_FAILED) {
 		trace_seq_printf(s, "[FAILED TO PARSE]");
@@ -5008,7 +5256,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
 
 		} else if (*ptr == '%') {
 			saveptr = ptr;
-			show_func = 0;
 			len_as_arg = 0;
  cont_process:
 			ptr++;
@@ -5046,39 +5293,19 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
 			case '-':
 				goto cont_process;
 			case 'p':
-				if (tep->long_size == 4)
-					ls = 1;
-				else
-					ls = 2;
-
-				if (isalnum(ptr[1]))
-					ptr++;
-
 				if (arg->type == TEP_PRINT_BSTRING) {
 					trace_seq_puts(s, arg->string.string);
 					arg = arg->next;
 					break;
 				}
-
-				if (*ptr == 'F' || *ptr == 'f' ||
-				    *ptr == 'S' || *ptr == 's') {
-					show_func = *ptr;
-				} else if (*ptr == 'M' || *ptr == 'm') {
-					print_mac_arg(s, *ptr, data, size, event, arg);
-					arg = arg->next;
-					break;
-				} else if (*ptr == 'I' || *ptr == 'i') {
-					int n;
-
-					n = print_ip_arg(s, ptr, data, size, event, arg);
-					if (n > 0) {
-						ptr += n - 1;
-						arg = arg->next;
-						break;
-					}
-				}
-
-				/* fall through */
+				ret = print_pointer(s, ptr + 1,
+						    len_as_arg ? len_arg : 1,
+						    data, size,
+						    event, arg);
+				arg = arg->next;
+				if (ret > 0)
+					ptr += ret;
+				break;
 			case 'd':
 			case 'u':
 			case 'i':
@@ -5107,17 +5334,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
 				val = eval_num_arg(data, size, event, arg);
 				arg = arg->next;
 
-				if (show_func) {
-					func = find_func(tep, val);
-					if (func) {
-						trace_seq_puts(s, func->func);
-						if (show_func == 'F')
-							trace_seq_printf(s,
-							       "+0x%llx",
-							       val - func->addr);
-						break;
-					}
-				}
 				if (tep->long_size == 8 && ls == 1 &&
 				    sizeof(long) != 8) {
 					char *p;
@@ -5125,8 +5341,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
 					/* make %l into %ll */
 					if (ls == 1 && (p = strchr(format, 'l')))
 						memmove(p+1, p, strlen(p)+1);
-					else if (strcmp(format, "%p") == 0)
-						strcpy(format, "0x%llx");
 					ls = 2;
 				}
 				switch (ls) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] trace-cmd: Add support for more printk format specifiers
  2020-05-12 15:51 [PATCH v2] trace-cmd: Add support for more printk format specifiers Tzvetomir Stoyanov (VMware)
@ 2020-05-13  1:14 ` Steven Rostedt
  2020-05-13  3:56   ` Tzvetomir Stoyanov
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2020-05-13  1:14 UTC (permalink / raw)
  To: Tzvetomir Stoyanov (VMware); +Cc: linux-trace-devel, johannes

On Tue, 12 May 2020 18:51:27 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> -static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
> +static int parse_ip4_print_args(struct tep_handle *tep,
> +				const char *ptr, bool *reverse)
> +{
> +	int ret = 0;
> +
> +	*reverse = false;
> +
> +	/* hnbl */
> +	switch (*ptr) {
> +	case 'h':
> +		if (tep->file_bigendian)
> +			*reverse = false;
> +		else
> +			*reverse = true;
> +		ret++;
> +	break;
> +	case 'l':
> +		*reverse = true;
> +		ret++;
> +	break;
> +	case 'n':
> +	case 'b':
> +		ret++;
> +		/* fall through */
> +	default:
> +		*reverse = false;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +static void print_ip4_addr(struct trace_seq *s, char i, bool reverse, unsigned char *buf)

Need a space between the end bracket and the next function.

>  {
>  	const char *fmt;
>  
> @@ -4555,7 +4604,11 @@ static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
>  	else
>  		fmt = "%d.%d.%d.%d";
>  
> -	trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
> +	if (reverse)
> +		trace_seq_printf(s, fmt, buf[3], buf[2], buf[1], buf[0]);
> +	else
> +		trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
> +
>  }
>  


> +int  guid_index[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15};
> +int  uuid_index[16] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
> +static int print_uuid_arg(struct trace_seq *s, const char *ptr,

Add a space between the variables and the function.

Also, make them static const:

static const int [gu]id_index[16] = ...

> +			void *data, int size, struct tep_event *event,
> +			struct tep_print_arg *arg)
> +{
> +	int *index = uuid_index;
> +	char *format = "%02x";
> +	int ret = 0;
> +	char *buf;
> +	int i;
> +
> +	switch (*(ptr + 1)) {
> +	case 'L':
> +		format = "%02X";
> +		/* fall through */
> +	case 'l':
> +		index = guid_index;
> +		ret++;
> +		break;
> +	case 'B':
> +		format = "%02X";
> +		/* fall through */
> +	case 'b':
> +		ret++;
> +		break;
> +	}
> +
> +	if (arg->type == TEP_PRINT_FUNC) {
> +		process_defined_func(s, data, size, event, arg);
> +		return ret;
> +	}
> +
> +	if (arg->type != TEP_PRINT_FIELD) {
> +		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
> +		return ret;
> +	}
> +
> +	if (!arg->field.field) {
> +		arg->field.field =
> +			tep_find_any_field(event, arg->field.name);
> +		if (!arg->field.field) {
> +			do_warning("%s: field %s not found",
> +				   __func__, arg->field.name);
> +			return ret;
> +		}
> +	}
> +
> +	if (arg->field.field->size != 16) {
> +		trace_seq_printf(s, "INVALIDUUID");
> +		return ret;
> +	}
> +
> +	buf = data + arg->field.field->offset;
> +
> +	for (i = 0; i < 16; i++) {
> +		trace_seq_printf(s, format, buf[index[i]] & 0xff);
> +		switch (i) {
> +		case 3:
> +		case 5:
> +		case 7:
> +		case 9:
> +			trace_seq_printf(s, "-");
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int print_raw_buff_arg(struct trace_seq *s, const char *ptr,
> +			      void *data, int size, struct tep_event *event,
> +			      struct tep_print_arg *arg, int print_len)
> +{
> +	int plen = print_len;
> +	char *delim = " ";
> +	int ret = 0;
> +	char *buf;
> +	int i;
> +	unsigned long offset;
> +	int arr_len;
> +
> +	switch (*(ptr + 1)) {
> +	case 'C':
> +		delim = ":";
> +		ret++;
> +		break;
> +	case 'D':
> +		delim = "-";
> +		ret++;
> +		break;
> +	case 'N':
> +		delim = "";
> +		ret++;
> +		break;
> +	}
> +
> +	if (arg->type == TEP_PRINT_FUNC) {
> +		process_defined_func(s, data, size, event, arg);
> +		return ret;
> +	}
> +
> +	if (arg->type != TEP_PRINT_DYNAMIC_ARRAY) {
> +		trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
> +		return ret;

Hmm, shouldn't this also be able to work with a constant array?

> +	}
> +
> +	offset = tep_read_number(event->tep,
> +				 data + arg->dynarray.field->offset,
> +				 arg->dynarray.field->size);
> +	arr_len = (unsigned long long)(offset >> 16);
> +	buf = data + (offset & 0xffff);
> +
> +	if (arr_len < plen)
> +		plen = arr_len;
> +
> +	if (plen < 1)
> +		return ret;
> +
> +	trace_seq_printf(s, "%02x", buf[0] & 0xff);
> +	for (i = 1; i < plen; i++)
> +		trace_seq_printf(s, "%s%02x", delim, buf[i] & 0xff);
> +
> +	return ret;
> +}
> +
>  static int is_printable_array(char *p, unsigned int len)
>  {
>  	unsigned int i;
> @@ -4952,6 +5136,71 @@ void tep_print_fields(struct trace_seq *s, void *data,
>  	}
>  }
>  
> +static int print_function(struct trace_seq *s, const char *format,
> +			  void *data, int size, struct tep_event *event,
> +			  struct tep_print_arg *arg)
> +{
> +	struct func_map *func;
> +	unsigned long long val;
> +
> +	val = eval_num_arg(data, size, event, arg);
> +	func = find_func(event->tep, val);
> +	if (func) {
> +		trace_seq_puts(s, func->func);
> +		if (*format == 'F' || *format == 'S')
> +			trace_seq_printf(s, "+0x%llx", val - func->addr);
> +	} else {
> +		if (event->tep->long_size == 4)
> +			trace_seq_printf(s, "0x%lx", (long)val);
> +		else
> +			trace_seq_printf(s, "0x%llx", (long long)val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int print_pointer(struct trace_seq *s, const char *format, int plen,
> +			 void *data, int size,
> +			 struct tep_event *event, struct tep_print_arg *arg)
> +{
> +	unsigned long long val;
> +	int ret = 1;
> +
> +	switch (*format) {
> +	case 'F':
> +	case 'f':
> +	case 'S':
> +	case 's':
> +		ret += print_function(s, format, data, size, event, arg);
> +		break;
> +	case 'M':
> +	case 'm':
> +		ret += print_mac_arg(s, format, data, size, event, arg);
> +		break;
> +	case 'I':
> +	case 'i':
> +		ret += print_ip_arg(s, format, data, size, event, arg);
> +		break;
> +	case 'U':
> +		ret += print_uuid_arg(s, format, data, size, event, arg);
> +		break;
> +	case 'h':
> +		ret += print_raw_buff_arg(s, format, data, size, event, arg, plen);
> +		break;
> +	default:
> +		ret = 0;
> +		val = eval_num_arg(data, size, event, arg);
> +		if (event->tep->long_size == 4)
> +			trace_seq_printf(s, "0x%lx", (long)val);
> +		else
> +			trace_seq_printf(s, "0x%llx", (long long)val);
> +		break;
> +	}
> +
> +	return ret;
> +
> +}
> +
>  static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_event *event)
>  {
>  	struct tep_handle *tep = event->tep;
> @@ -4960,16 +5209,15 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
>  	struct tep_print_arg *args = NULL;
>  	const char *ptr = print_fmt->format;
>  	unsigned long long val;
> -	struct func_map *func;
>  	const char *saveptr;
>  	struct trace_seq p;
>  	char *bprint_fmt = NULL;
>  	char format[32];
> -	int show_func;
>  	int len_as_arg;
>  	int len_arg = 0;
>  	int len;
>  	int ls;
> +	int ret;
>  
>  	if (event->flags & TEP_EVENT_FL_FAILED) {
>  		trace_seq_printf(s, "[FAILED TO PARSE]");
> @@ -5008,7 +5256,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
>  
>  		} else if (*ptr == '%') {
>  			saveptr = ptr;
> -			show_func = 0;
>  			len_as_arg = 0;
>   cont_process:
>  			ptr++;
> @@ -5046,39 +5293,19 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
>  			case '-':
>  				goto cont_process;
>  			case 'p':
> -				if (tep->long_size == 4)
> -					ls = 1;
> -				else
> -					ls = 2;
> -
> -				if (isalnum(ptr[1]))
> -					ptr++;
> -
>  				if (arg->type == TEP_PRINT_BSTRING) {

This will still need the above check. As this is for trace_printk() in
the kernel. And that will record the pointer conversion (except for
functions) into the buffer as a string (hence why it only prints the
string). But that said, the format still needs to be passed, otherwise
it is printed. We should (not in this patch), probably do the full
logic of parsing the format as well, as the check for isalnum() is not
enough.

-- Steve


>  					trace_seq_puts(s, arg->string.string);
>  					arg = arg->next;
>  					break;
>  				}
> -
> -				if (*ptr == 'F' || *ptr == 'f' ||
> -				    *ptr == 'S' || *ptr == 's') {
> -					show_func = *ptr;
> -				} else if (*ptr == 'M' || *ptr == 'm') {
> -					print_mac_arg(s, *ptr, data, size, event, arg);
> -					arg = arg->next;
> -					break;
> -				} else if (*ptr == 'I' || *ptr == 'i') {
> -					int n;
> -
> -					n = print_ip_arg(s, ptr, data, size, event, arg);
> -					if (n > 0) {
> -						ptr += n - 1;
> -						arg = arg->next;
> -						break;
> -					}
> -				}
> -
> -				/* fall through */
> +				ret = print_pointer(s, ptr + 1,
> +						    len_as_arg ? len_arg : 1,
> +						    data, size,
> +						    event, arg);
> +				arg = arg->next;
> +				if (ret > 0)
> +					ptr += ret;
> +				break;
>  			case 'd':
>  			case 'u':
>  			case 'i':
> @@ -5107,17 +5334,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
>  				val = eval_num_arg(data, size, event, arg);
>  				arg = arg->next;
>  
> -				if (show_func) {
> -					func = find_func(tep, val);
> -					if (func) {
> -						trace_seq_puts(s, func->func);
> -						if (show_func == 'F')
> -							trace_seq_printf(s,
> -							       "+0x%llx",
> -							       val - func->addr);
> -						break;
> -					}
> -				}
>  				if (tep->long_size == 8 && ls == 1 &&
>  				    sizeof(long) != 8) {
>  					char *p;
> @@ -5125,8 +5341,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
>  					/* make %l into %ll */
>  					if (ls == 1 && (p = strchr(format, 'l')))
>  						memmove(p+1, p, strlen(p)+1);
> -					else if (strcmp(format, "%p") == 0)
> -						strcpy(format, "0x%llx");
>  					ls = 2;
>  				}
>  				switch (ls) {


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] trace-cmd: Add support for more printk format specifiers
  2020-05-13  1:14 ` Steven Rostedt
@ 2020-05-13  3:56   ` Tzvetomir Stoyanov
  0 siblings, 0 replies; 3+ messages in thread
From: Tzvetomir Stoyanov @ 2020-05-13  3:56 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel, johannes

On Wed, May 13, 2020 at 4:14 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 12 May 2020 18:51:27 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > -static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
> > +static int parse_ip4_print_args(struct tep_handle *tep,
> > +                             const char *ptr, bool *reverse)
> > +{
> > +     int ret = 0;
> > +
> > +     *reverse = false;
> > +
> > +     /* hnbl */
> > +     switch (*ptr) {
> > +     case 'h':
> > +             if (tep->file_bigendian)
> > +                     *reverse = false;
> > +             else
> > +                     *reverse = true;
> > +             ret++;
> > +     break;
> > +     case 'l':
> > +             *reverse = true;
> > +             ret++;
> > +     break;
> > +     case 'n':
> > +     case 'b':
> > +             ret++;
> > +             /* fall through */
> > +     default:
> > +             *reverse = false;
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +}
> > +static void print_ip4_addr(struct trace_seq *s, char i, bool reverse, unsigned char *buf)
>
> Need a space between the end bracket and the next function.
>
> >  {
> >       const char *fmt;
> >
> > @@ -4555,7 +4604,11 @@ static void print_ip4_addr(struct trace_seq *s, char i, unsigned char *buf)
> >       else
> >               fmt = "%d.%d.%d.%d";
> >
> > -     trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
> > +     if (reverse)
> > +             trace_seq_printf(s, fmt, buf[3], buf[2], buf[1], buf[0]);
> > +     else
> > +             trace_seq_printf(s, fmt, buf[0], buf[1], buf[2], buf[3]);
> > +
> >  }
> >
>
>
> > +int  guid_index[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15};
> > +int  uuid_index[16] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
> > +static int print_uuid_arg(struct trace_seq *s, const char *ptr,
>
> Add a space between the variables and the function.
>
> Also, make them static const:
>
> static const int [gu]id_index[16] = ...
>
> > +                     void *data, int size, struct tep_event *event,
> > +                     struct tep_print_arg *arg)
> > +{
> > +     int *index = uuid_index;
> > +     char *format = "%02x";
> > +     int ret = 0;
> > +     char *buf;
> > +     int i;
> > +
> > +     switch (*(ptr + 1)) {
> > +     case 'L':
> > +             format = "%02X";
> > +             /* fall through */
> > +     case 'l':
> > +             index = guid_index;
> > +             ret++;
> > +             break;
> > +     case 'B':
> > +             format = "%02X";
> > +             /* fall through */
> > +     case 'b':
> > +             ret++;
> > +             break;
> > +     }
> > +
> > +     if (arg->type == TEP_PRINT_FUNC) {
> > +             process_defined_func(s, data, size, event, arg);
> > +             return ret;
> > +     }
> > +
> > +     if (arg->type != TEP_PRINT_FIELD) {
> > +             trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
> > +             return ret;
> > +     }
> > +
> > +     if (!arg->field.field) {
> > +             arg->field.field =
> > +                     tep_find_any_field(event, arg->field.name);
> > +             if (!arg->field.field) {
> > +                     do_warning("%s: field %s not found",
> > +                                __func__, arg->field.name);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     if (arg->field.field->size != 16) {
> > +             trace_seq_printf(s, "INVALIDUUID");
> > +             return ret;
> > +     }
> > +
> > +     buf = data + arg->field.field->offset;
> > +
> > +     for (i = 0; i < 16; i++) {
> > +             trace_seq_printf(s, format, buf[index[i]] & 0xff);
> > +             switch (i) {
> > +             case 3:
> > +             case 5:
> > +             case 7:
> > +             case 9:
> > +                     trace_seq_printf(s, "-");
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int print_raw_buff_arg(struct trace_seq *s, const char *ptr,
> > +                           void *data, int size, struct tep_event *event,
> > +                           struct tep_print_arg *arg, int print_len)
> > +{
> > +     int plen = print_len;
> > +     char *delim = " ";
> > +     int ret = 0;
> > +     char *buf;
> > +     int i;
> > +     unsigned long offset;
> > +     int arr_len;
> > +
> > +     switch (*(ptr + 1)) {
> > +     case 'C':
> > +             delim = ":";
> > +             ret++;
> > +             break;
> > +     case 'D':
> > +             delim = "-";
> > +             ret++;
> > +             break;
> > +     case 'N':
> > +             delim = "";
> > +             ret++;
> > +             break;
> > +     }
> > +
> > +     if (arg->type == TEP_PRINT_FUNC) {
> > +             process_defined_func(s, data, size, event, arg);
> > +             return ret;
> > +     }
> > +
> > +     if (arg->type != TEP_PRINT_DYNAMIC_ARRAY) {
> > +             trace_seq_printf(s, "ARG TYPE NOT FIELD BUT %d", arg->type);
> > +             return ret;
>
> Hmm, shouldn't this also be able to work with a constant array?

I should work, but I couldn't find any events using this print format
with other than dynamic array,
in order to test it. We can add other argument types when I implement
a test libtraceevent kernel
module, so we can test any combinations between print format strings
and argument types.

>
> > +     }
> > +
> > +     offset = tep_read_number(event->tep,
> > +                              data + arg->dynarray.field->offset,
> > +                              arg->dynarray.field->size);
> > +     arr_len = (unsigned long long)(offset >> 16);
> > +     buf = data + (offset & 0xffff);
> > +
> > +     if (arr_len < plen)
> > +             plen = arr_len;
> > +
> > +     if (plen < 1)
> > +             return ret;
> > +
> > +     trace_seq_printf(s, "%02x", buf[0] & 0xff);
> > +     for (i = 1; i < plen; i++)
> > +             trace_seq_printf(s, "%s%02x", delim, buf[i] & 0xff);
> > +
> > +     return ret;
> > +}
> > +
> >  static int is_printable_array(char *p, unsigned int len)
> >  {
> >       unsigned int i;
> > @@ -4952,6 +5136,71 @@ void tep_print_fields(struct trace_seq *s, void *data,
> >       }
> >  }
> >
> > +static int print_function(struct trace_seq *s, const char *format,
> > +                       void *data, int size, struct tep_event *event,
> > +                       struct tep_print_arg *arg)
> > +{
> > +     struct func_map *func;
> > +     unsigned long long val;
> > +
> > +     val = eval_num_arg(data, size, event, arg);
> > +     func = find_func(event->tep, val);
> > +     if (func) {
> > +             trace_seq_puts(s, func->func);
> > +             if (*format == 'F' || *format == 'S')
> > +                     trace_seq_printf(s, "+0x%llx", val - func->addr);
> > +     } else {
> > +             if (event->tep->long_size == 4)
> > +                     trace_seq_printf(s, "0x%lx", (long)val);
> > +             else
> > +                     trace_seq_printf(s, "0x%llx", (long long)val);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int print_pointer(struct trace_seq *s, const char *format, int plen,
> > +                      void *data, int size,
> > +                      struct tep_event *event, struct tep_print_arg *arg)
> > +{
> > +     unsigned long long val;
> > +     int ret = 1;
> > +
> > +     switch (*format) {
> > +     case 'F':
> > +     case 'f':
> > +     case 'S':
> > +     case 's':
> > +             ret += print_function(s, format, data, size, event, arg);
> > +             break;
> > +     case 'M':
> > +     case 'm':
> > +             ret += print_mac_arg(s, format, data, size, event, arg);
> > +             break;
> > +     case 'I':
> > +     case 'i':
> > +             ret += print_ip_arg(s, format, data, size, event, arg);
> > +             break;
> > +     case 'U':
> > +             ret += print_uuid_arg(s, format, data, size, event, arg);
> > +             break;
> > +     case 'h':
> > +             ret += print_raw_buff_arg(s, format, data, size, event, arg, plen);
> > +             break;
> > +     default:
> > +             ret = 0;
> > +             val = eval_num_arg(data, size, event, arg);
> > +             if (event->tep->long_size == 4)
> > +                     trace_seq_printf(s, "0x%lx", (long)val);
> > +             else
> > +                     trace_seq_printf(s, "0x%llx", (long long)val);
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +
> > +}
> > +
> >  static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_event *event)
> >  {
> >       struct tep_handle *tep = event->tep;
> > @@ -4960,16 +5209,15 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
> >       struct tep_print_arg *args = NULL;
> >       const char *ptr = print_fmt->format;
> >       unsigned long long val;
> > -     struct func_map *func;
> >       const char *saveptr;
> >       struct trace_seq p;
> >       char *bprint_fmt = NULL;
> >       char format[32];
> > -     int show_func;
> >       int len_as_arg;
> >       int len_arg = 0;
> >       int len;
> >       int ls;
> > +     int ret;
> >
> >       if (event->flags & TEP_EVENT_FL_FAILED) {
> >               trace_seq_printf(s, "[FAILED TO PARSE]");
> > @@ -5008,7 +5256,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
> >
> >               } else if (*ptr == '%') {
> >                       saveptr = ptr;
> > -                     show_func = 0;
> >                       len_as_arg = 0;
> >   cont_process:
> >                       ptr++;
> > @@ -5046,39 +5293,19 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
> >                       case '-':
> >                               goto cont_process;
> >                       case 'p':
> > -                             if (tep->long_size == 4)
> > -                                     ls = 1;
> > -                             else
> > -                                     ls = 2;
> > -
> > -                             if (isalnum(ptr[1]))
> > -                                     ptr++;
> > -
> >                               if (arg->type == TEP_PRINT_BSTRING) {
>
> This will still need the above check. As this is for trace_printk() in
> the kernel. And that will record the pointer conversion (except for
> functions) into the buffer as a string (hence why it only prints the
> string). But that said, the format still needs to be passed, otherwise
> it is printed. We should (not in this patch), probably do the full
> logic of parsing the format as well, as the check for isalnum() is not
> enough.
>
> -- Steve

Thanks, Steven! I'll submit the next patch version addressing these comments.

>
>
> >                                       trace_seq_puts(s, arg->string.string);
> >                                       arg = arg->next;
> >                                       break;
> >                               }
> > -
> > -                             if (*ptr == 'F' || *ptr == 'f' ||
> > -                                 *ptr == 'S' || *ptr == 's') {
> > -                                     show_func = *ptr;
> > -                             } else if (*ptr == 'M' || *ptr == 'm') {
> > -                                     print_mac_arg(s, *ptr, data, size, event, arg);
> > -                                     arg = arg->next;
> > -                                     break;
> > -                             } else if (*ptr == 'I' || *ptr == 'i') {
> > -                                     int n;
> > -
> > -                                     n = print_ip_arg(s, ptr, data, size, event, arg);
> > -                                     if (n > 0) {
> > -                                             ptr += n - 1;
> > -                                             arg = arg->next;
> > -                                             break;
> > -                                     }
> > -                             }
> > -
> > -                             /* fall through */
> > +                             ret = print_pointer(s, ptr + 1,
> > +                                                 len_as_arg ? len_arg : 1,
> > +                                                 data, size,
> > +                                                 event, arg);
> > +                             arg = arg->next;
> > +                             if (ret > 0)
> > +                                     ptr += ret;
> > +                             break;
> >                       case 'd':
> >                       case 'u':
> >                       case 'i':
> > @@ -5107,17 +5334,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
> >                               val = eval_num_arg(data, size, event, arg);
> >                               arg = arg->next;
> >
> > -                             if (show_func) {
> > -                                     func = find_func(tep, val);
> > -                                     if (func) {
> > -                                             trace_seq_puts(s, func->func);
> > -                                             if (show_func == 'F')
> > -                                                     trace_seq_printf(s,
> > -                                                            "+0x%llx",
> > -                                                            val - func->addr);
> > -                                             break;
> > -                                     }
> > -                             }
> >                               if (tep->long_size == 8 && ls == 1 &&
> >                                   sizeof(long) != 8) {
> >                                       char *p;
> > @@ -5125,8 +5341,6 @@ static void pretty_print(struct trace_seq *s, void *data, int size, struct tep_e
> >                                       /* make %l into %ll */
> >                                       if (ls == 1 && (p = strchr(format, 'l')))
> >                                               memmove(p+1, p, strlen(p)+1);
> > -                                     else if (strcmp(format, "%p") == 0)
> > -                                             strcpy(format, "0x%llx");
> >                                       ls = 2;
> >                               }
> >                               switch (ls) {
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 15:51 [PATCH v2] trace-cmd: Add support for more printk format specifiers Tzvetomir Stoyanov (VMware)
2020-05-13  1:14 ` Steven Rostedt
2020-05-13  3:56   ` Tzvetomir Stoyanov

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git