[PATCH 0/3] Switch V3 messages to text encoding of variable length buffer

[PATCH 0/3] Switch V3 messages to text encoding of variable length buffer

[Slavomir Kaslev]

This patchset switches protocol V3 messages to use text encoding for the
variable length buffer at the end as Steven suggested.

Slavomir Kaslev (3):
  trace-cmd: Use text encoding for options in protocol V3
  trace-cmd: Use text encoding for transmitting ports in protocol V3
  trace-cmd: Add msg_buf_len() function

 tracecmd/trace-msg.c | 166 ++++++++++++++++++++++++++-----------------
 1 file changed, 99 insertions(+), 67 deletions(-)

-- 
2.19.1

[PATCH 1/3] trace-cmd: Use text encoding for options in protocol V3

[Slavomir Kaslev]

Options are now encoded as text in the buffer at the end of protocol V3 messages.

Unrecognized options are logged and ignored by the listener to support adding
new options in the future.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 tracecmd/trace-msg.c | 82 +++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 50 deletions(-)

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 51d0ac8..4b43849 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -49,11 +49,6 @@ static inline void dprint(const char *fmt, ...)
 
 unsigned int page_size;
 
-struct tracecmd_msg_opt {
-	be32 size;
-	be32 opt_cmd;
-} __attribute__((packed));
-
 struct tracecmd_msg_tinit {
 	be32 cpus;
 	be32 page_size;
@@ -110,7 +105,6 @@ struct tracecmd_msg {
 		struct tracecmd_msg_rinit	rinit;
 	};
 	union {
-		struct tracecmd_msg_opt		*opt;
 		be32				*port_array;
 		void				*buf;
 	};
@@ -143,27 +137,17 @@ static int msg_write(int fd, struct tracecmd_msg *msg)
 	return __do_write_check(fd, msg->buf, data_size);
 }
 
-enum msg_opt_command {
-	MSGOPT_USETCP = 1,
-};
-
 static int make_tinit(struct tracecmd_msg_handle *msg_handle,
 		      struct tracecmd_msg *msg)
 {
-	struct tracecmd_msg_opt *opt;
 	int cpu_count = msg_handle->cpu_count;
 	int opt_num = 0;
 	int data_size = 0;
 
 	if (msg_handle->flags & TRACECMD_MSG_FL_USE_TCP) {
 		opt_num++;
-		opt = malloc(sizeof(*opt));
-		if (!opt)
-			return -ENOMEM;
-		opt->size = htonl(sizeof(*opt));
-		opt->opt_cmd = htonl(MSGOPT_USETCP);
-		msg->opt = opt;
-		data_size += sizeof(*opt);
+		msg->buf = strdup("tcp");
+		data_size += 4;
 	}
 
 	msg->tinit.cpus = htonl(cpu_count);
@@ -441,10 +425,10 @@ out:
 }
 
 static bool process_option(struct tracecmd_msg_handle *msg_handle,
-			   struct tracecmd_msg_opt *opt)
+			   const char *opt)
 {
-	/* currently the only option we have is to us TCP */
-	if (ntohl(opt->opt_cmd) == MSGOPT_USETCP) {
+	/* currently the only option we have is to use TCP */
+	if (strcmp(opt, "tcp") == 0) {
 		msg_handle->flags |= TRACECMD_MSG_FL_USE_TCP;
 		return true;
 	}
@@ -475,15 +459,15 @@ void tracecmd_msg_handle_close(struct tracecmd_msg_handle *msg_handle)
 
 int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 {
-	struct tracecmd_msg_opt *opt;
 	struct tracecmd_msg msg;
+	char *p, *buf_end;
+	ssize_t buf_len;
 	int pagesize;
-	int options, i, s;
+	int options, i;
 	int cpus;
 	int ret;
-	int offset = 0;
-	u32 size;
 
+	memset(&msg, 0, sizeof(msg));
 	ret = tracecmd_msg_recv_wait(msg_handle->fd, &msg);
 	if (ret < 0) {
 		if (ret == -ETIMEDOUT)
@@ -512,38 +496,36 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 		goto error;
 	}
 
-	size = MSG_HDR_LEN + ntohl(msg.hdr.cmd_size);
+	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	if (buf_len < 0) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	if (buf_len == 0)
+		goto no_options;
+
+	if (((char *)msg.buf)[buf_len-1] != '\0') {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	buf_end = (char *)msg.buf + buf_len;
 	options = ntohl(msg.tinit.opt_num);
-	for (i = 0; i < options; i++) {
-		if (size + sizeof(*opt) > ntohl(msg.hdr.size)) {
-			plog("Not enough message for options\n");
+	for (i = 0, p = msg.buf; i < options; i++, p++) {
+		if (p >= buf_end) {
 			ret = -EINVAL;
 			goto error;
 		}
-		opt = (void *)msg.opt + offset;
-		offset += ntohl(opt->size);
-		size += ntohl(opt->size);
-		if (ntohl(msg.hdr.size) < size) {
-			plog("Not enough message for options\n");
-			ret = -EINVAL;
-			goto error;
-		}
-		/* prevent a client from killing us */
-		if (ntohl(opt->size) > MAX_OPTION_SIZE) {
-			plog("Exceed MAX_OPTION_SIZE\n");
-			ret = -EINVAL;
-			goto error;
-		}
-		s = process_option(msg_handle, opt);
+
 		/* do we understand this option? */
-		if (!s) {
-			plog("Cannot understand(%d:%d:%d)\n",
-			     i, ntohl(opt->size), ntohl(opt->opt_cmd));
-			ret = -EINVAL;
-			goto error;
-		}
+		if (!process_option(msg_handle, p))
+			plog("Cannot understand option '%s'\n", p);
+
+		p = strchr(p, '\0');
 	}
 
+no_options:
 	msg_free(&msg);
 	return pagesize;
 
-- 
2.19.1

[PATCH 2/3] trace-cmd: Use text encoding for transmitting ports in protocol V3

[Slavomir Kaslev]

Ports are now encoded as text in the buffer at the end of protocol V3 messages.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 tracecmd/trace-msg.c | 77 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 4b43849..48f1eac 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -104,10 +104,7 @@ struct tracecmd_msg {
 		struct tracecmd_msg_tinit	tinit;
 		struct tracecmd_msg_rinit	rinit;
 	};
-	union {
-		be32				*port_array;
-		void				*buf;
-	};
+	void					*buf;
 } __attribute__((packed));
 
 static int msg_write(int fd, struct tracecmd_msg *msg)
@@ -159,19 +156,41 @@ static int make_tinit(struct tracecmd_msg_handle *msg_handle,
 	return 0;
 }
 
+static int write_ints(char *buf, size_t buf_len, int *arr, int arr_len)
+{
+	int i, ret, tot = 0;
+
+	for (i = 0; i < arr_len; i++) {
+		ret = snprintf(buf, buf_len, "%d", arr[i]);
+		if (ret < 0)
+			return ret;
+
+		/* Count the '\0' byte */
+		ret++;
+		tot += ret;
+		if (buf)
+			buf += ret;
+		if (buf_len >= ret)
+			buf_len -= ret;
+		else
+			buf_len = 0;
+	}
+
+	return tot;
+}
+
 static int make_rinit(struct tracecmd_msg *msg, int cpus, int *ports)
 {
-	int i;
+	int data_size;
 
-	msg->rinit.cpus = htonl(cpus);
-	msg->port_array = malloc(sizeof(*ports) * cpus);
-	if (!msg->port_array)
+	data_size = write_ints(NULL, 0, ports, cpus);
+	msg->buf = malloc(data_size);
+	if (!msg->buf)
 		return -ENOMEM;
+	write_ints(msg->buf, data_size, ports, cpus);
 
-	for (i = 0; i < cpus; i++)
-		msg->port_array[i] = htonl(ports[i]);
-
-	msg->hdr.size = htonl(ntohl(msg->hdr.size) + sizeof(*ports) * cpus);
+	msg->rinit.cpus = htonl(cpus);
+	msg->hdr.size = htonl(ntohl(msg->hdr.size) + data_size);
 
 	return 0;
 }
@@ -380,8 +399,9 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 	struct tracecmd_msg msg;
 	int fd = msg_handle->fd;
 	unsigned int *ports;
-	int i, cpus;
-	int ret;
+	int i, cpus, ret;
+	char *p, *buf_end;
+	ssize_t buf_len;
 
 	*client_ports = NULL;
 
@@ -405,10 +425,35 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 		goto error;
 	}
 
+	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	if (buf_len <= 0) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	if (((char *)msg.buf)[buf_len-1] != '\0') {
+		ret = -EINVAL;
+		goto error;
+	}
+
 	cpus = ntohl(msg.rinit.cpus);
 	ports = malloc_or_die(sizeof(*ports) * cpus);
-	for (i = 0; i < cpus; i++)
-		ports[i] = ntohl(msg.port_array[i]);
+	if (!ports) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	buf_end = (char *)msg.buf + buf_len;
+	for (i = 0, p = msg.buf; i < cpus; i++, p++) {
+		if (p >= buf_end) {
+			free(ports);
+			ret = -EINVAL;
+			goto error;
+		}
+
+		ports[i] = atoi(p);
+		p = strchr(p, '\0');
+	}
 
 	*client_ports = ports;
 
-- 
2.19.1

[PATCH 3/3] trace-cmd: Add msg_buf_len() function

[Slavomir Kaslev]

Factor out common code computing message buffer length in a function.

No changes in behavior intended.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 tracecmd/trace-msg.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 48f1eac..a91b211 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -107,6 +107,11 @@ struct tracecmd_msg {
 	void					*buf;
 } __attribute__((packed));
 
+static int msg_buf_len(struct tracecmd_msg *msg)
+{
+	return ntohl(msg->hdr.size) - MSG_HDR_LEN - ntohl(msg->hdr.cmd_size);
+}
+
 static int msg_write(int fd, struct tracecmd_msg *msg)
 {
 	int cmd = ntohl(msg->hdr.cmd);
@@ -425,7 +430,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 		goto error;
 	}
 
-	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	buf_len = msg_buf_len(&msg);
 	if (buf_len <= 0) {
 		ret = -EINVAL;
 		goto error;
@@ -541,7 +546,7 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 		goto error;
 	}
 
-	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	buf_len = msg_buf_len(&msg);
 	if (buf_len < 0) {
 		ret = -EINVAL;
 		goto error;
@@ -685,7 +690,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 			goto next;
 		}
 
-		n = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+		n = msg_buf_len(&msg);
 		t = n;
 		s = 0;
 		while (t > 0) {
-- 
2.19.1

Re: [PATCH 3/3] trace-cmd: Add msg_buf_len() function

[Steven Rostedt]

On Fri, 15 Mar 2019 17:33:26 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> Factor out common code computing message buffer length in a function.
> 
> No changes in behavior intended.
> 
> Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
> ---
>  tracecmd/trace-msg.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
> index 48f1eac..a91b211 100644
> --- a/tracecmd/trace-msg.c
> +++ b/tracecmd/trace-msg.c
> @@ -107,6 +107,11 @@ struct tracecmd_msg {
>  	void					*buf;
>  } __attribute__((packed));
>  
> +static int msg_buf_len(struct tracecmd_msg *msg)

I changed this to:

static inline int msg_buf_len(...)

-- Steve

> +{
> +	return ntohl(msg->hdr.size) - MSG_HDR_LEN - ntohl(msg->hdr.cmd_size);
> +}
> +
>  static int msg_write(int fd, struct tracecmd_msg *msg)
>  {
>  	int cmd = ntohl(msg->hdr.cmd);
> @@ -425,7 +430,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
>  		goto error;
>  	}
>  
> -	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> +	buf_len = msg_buf_len(&msg);
>  	if (buf_len <= 0) {
>  		ret = -EINVAL;
>  		goto error;
> @@ -541,7 +546,7 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
>  		goto error;
>  	}
>  
> -	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> +	buf_len = msg_buf_len(&msg);
>  	if (buf_len < 0) {
>  		ret = -EINVAL;
>  		goto error;
> @@ -685,7 +690,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
>  			goto next;
>  		}
>  
> -		n = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> +		n = msg_buf_len(&msg);
>  		t = n;
>  		s = 0;
>  		while (t > 0) {