diff mbox series

[RFC,v8,05/13] trace-cmd: Add TRACE_REQ and TRACE_RESP messages

Message ID 20190222180539.27439-6-kaslevs@vmware.com (mailing list archive)
State Superseded
Headers show
Series Add VM kernel tracing over vsockets and FIFOs | expand

Commit Message

Slavomir Kaslev Feb. 22, 2019, 6:05 p.m. UTC 
Add TRACE_REQ and TRACE_RESP messages which are used for initiating guest VM
tracing.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 include/trace-cmd/trace-cmd.h |  12 ++
 tracecmd/trace-msg.c          | 211 +++++++++++++++++++++++++++++++++-
 2 files changed, 222 insertions(+), 1 deletion(-)

Comments

Steven Rostedt Feb. 22, 2019, 9:09 p.m. UTC | #1 
On Fri, 22 Feb 2019 20:05:31 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> +static int make_trace_resp(struct tracecmd_msg *msg,
> +			   int page_size, int nr_cpus, unsigned int *ports)

I wonder if we should just have a generic "argc"/"argv" protocol to
send dynamic amounts of data, and not have specific dynamic messages?

Perhaps pass the numbers as ASCII? And then converted them at the other
end. We need to handle ntoh anyway, so a conversion (albeit a fast one)
is probably done anyway.

We could send:
 int argc =  6;
 char **argv = { "4096", "16", "33445", "33446", "33448", "33449" };

Or, if you are not comfortable with sending ASCII and converting, what
about have a specific "sending an array of numbers"?

 int argc = 6;
 int *argv = { 4096, 16, 33445, 33446, 33448, 33449 };

Thoughts?

-- Steve

> +{
> +	int ports_size = nr_cpus * sizeof(*msg->port_array);
> +	int i;
> +
> +	msg->hdr.size = htonl(ntohl(msg->hdr.size) + ports_size);
> +	msg->trace_resp.cpus = htonl(nr_cpus);
> +	msg->trace_resp.page_size = htonl(page_size);
> +
> +	msg->port_array = malloc(ports_size);
> +	if (!msg->port_array)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_cpus; i++)
> +		msg->port_array[i] = htonl(ports[i]);
> +
> +	return 0;
> +}
> +
> +int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
> +				 int nr_cpus, int page_size,
> +				 unsigned int *ports)
> +{
> +	struct tracecmd_msg msg;
> +	int ret;
> +
> +	tracecmd_msg_init(MSG_TRACE_RESP, &msg);
> +	ret = make_trace_resp(&msg, page_size, nr_cpus, ports);
> +	if (ret < 0)
> +		return ret;
> +
> +	return tracecmd_msg_send(msg_handle->fd, &msg);
> +}
> +
> +int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
> +				 int *nr_cpus, int *page_size,
> +				 unsigned int **ports)
> +{
> +	struct tracecmd_msg msg;
> +	ssize_t buf_len;
> +	int i, ret;
> +
> +	ret = tracecmd_msg_recv(msg_handle->fd, &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ntohl(msg.hdr.cmd) != MSG_TRACE_RESP) {
> +		ret = -ENOTSUP;
> +		goto out;
> +	}
> +
> +	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> +	if (buf_len <= 0 ||
> +	    buf_len != sizeof(*msg.port_array) * ntohl(msg.trace_resp.cpus)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	*nr_cpus = ntohl(msg.trace_resp.cpus);
> +	*page_size = ntohl(msg.trace_resp.page_size);
> +	*ports = calloc(*nr_cpus, sizeof(**ports));
> +	if (!*ports) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	for (i = 0; i < *nr_cpus; i++)
> +		(*ports)[i] = ntohl(msg.port_array[i]);
> +
> +	msg_free(&msg);
> +	return 0;
> +
> +out:
> +	error_operation(&msg);
> +	if (ret == -EOPNOTSUPP)
> +		handle_unexpected_msg(msg_handle, &msg);
> +	msg_free(&msg);
> +	return ret;
> +}
Slavomir Kaslev March 12, 2019, 6:48 p.m. UTC | #2 
On Fri, Feb 22, 2019 at 11:09 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 22 Feb 2019 20:05:31 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> > +static int make_trace_resp(struct tracecmd_msg *msg,
> > +                        int page_size, int nr_cpus, unsigned int *ports)
>
> I wonder if we should just have a generic "argc"/"argv" protocol to
> send dynamic amounts of data, and not have specific dynamic messages?
>
> Perhaps pass the numbers as ASCII? And then converted them at the other
> end. We need to handle ntoh anyway, so a conversion (albeit a fast one)
> is probably done anyway.
>
> We could send:
>  int argc =  6;
>  char **argv = { "4096", "16", "33445", "33446", "33448", "33449" };
>
> Or, if you are not comfortable with sending ASCII and converting, what
> about have a specific "sending an array of numbers"?
>
>  int argc = 6;
>  int *argv = { 4096, 16, 33445, 33446, 33448, 33449 };
>
> Thoughts?

I don't see any drawbacks of this approach (modulo a bit of
arithmetic). I implemented it the way it is following how the listener
is sending port numbers back to the recording process.

Should we also switch the listener to use the same encoding of ports
with protocol V3 before the next release?

How about options (which is the only other non-text use of the
variable buffer part of messages)? Currently the only option in use is
MSGOPT_USETCP and can become simply "tcp" on the wire.

We can then have a convention that the buffer at then end of messages
is always text and drop the second union in tracecmd_msg.

-- Slavi

>
> -- Steve
>
> > +{
> > +     int ports_size = nr_cpus * sizeof(*msg->port_array);
> > +     int i;
> > +
> > +     msg->hdr.size = htonl(ntohl(msg->hdr.size) + ports_size);
> > +     msg->trace_resp.cpus = htonl(nr_cpus);
> > +     msg->trace_resp.page_size = htonl(page_size);
> > +
> > +     msg->port_array = malloc(ports_size);
> > +     if (!msg->port_array)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < nr_cpus; i++)
> > +             msg->port_array[i] = htonl(ports[i]);
> > +
> > +     return 0;
> > +}
> > +
> > +int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
> > +                              int nr_cpus, int page_size,
> > +                              unsigned int *ports)
> > +{
> > +     struct tracecmd_msg msg;
> > +     int ret;
> > +
> > +     tracecmd_msg_init(MSG_TRACE_RESP, &msg);
> > +     ret = make_trace_resp(&msg, page_size, nr_cpus, ports);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return tracecmd_msg_send(msg_handle->fd, &msg);
> > +}
> > +
> > +int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
> > +                              int *nr_cpus, int *page_size,
> > +                              unsigned int **ports)
> > +{
> > +     struct tracecmd_msg msg;
> > +     ssize_t buf_len;
> > +     int i, ret;
> > +
> > +     ret = tracecmd_msg_recv(msg_handle->fd, &msg);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (ntohl(msg.hdr.cmd) != MSG_TRACE_RESP) {
> > +             ret = -ENOTSUP;
> > +             goto out;
> > +     }
> > +
> > +     buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> > +     if (buf_len <= 0 ||
> > +         buf_len != sizeof(*msg.port_array) * ntohl(msg.trace_resp.cpus)) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     *nr_cpus = ntohl(msg.trace_resp.cpus);
> > +     *page_size = ntohl(msg.trace_resp.page_size);
> > +     *ports = calloc(*nr_cpus, sizeof(**ports));
> > +     if (!*ports) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +     for (i = 0; i < *nr_cpus; i++)
> > +             (*ports)[i] = ntohl(msg.port_array[i]);
> > +
> > +     msg_free(&msg);
> > +     return 0;
> > +
> > +out:
> > +     error_operation(&msg);
> > +     if (ret == -EOPNOTSUPP)
> > +             handle_unexpected_msg(msg_handle, &msg);
> > +     msg_free(&msg);
> > +     return ret;
> > +}
>
Steven Rostedt March 12, 2019, 7:18 p.m. UTC | #3 
On Tue, 12 Mar 2019 18:48:29 +0000
Slavomir Kaslev <kaslevs@vmware.com> wrote:


> > We could send:
> >  int argc =  6;
> >  char **argv = { "4096", "16", "33445", "33446", "33448", "33449" };
> >
> > Or, if you are not comfortable with sending ASCII and converting, what
> > about have a specific "sending an array of numbers"?
> >
> >  int argc = 6;
> >  int *argv = { 4096, 16, 33445, 33446, 33448, 33449 };
> >
> > Thoughts?  
> 
> I don't see any drawbacks of this approach (modulo a bit of
> arithmetic). I implemented it the way it is following how the listener
> is sending port numbers back to the recording process.
> 
> Should we also switch the listener to use the same encoding of ports
> with protocol V3 before the next release?

Yeah, I think we can do that.

> 
> How about options (which is the only other non-text use of the
> variable buffer part of messages)? Currently the only option in use is
> MSGOPT_USETCP and can become simply "tcp" on the wire.
> 
> We can then have a convention that the buffer at then end of messages
> is always text and drop the second union in tracecmd_msg.
> 

Sure.

Do you think there's any use to keeping the "flags" field for the args?

I was thinking that we should rename trace_req to trace_args and add a
"type" field that can be used to verify the type of arguments being
sent. 

enum {
	TRACE_ARGS_PORTS	= 1,
	TRACE_ARGS_OPTIONS,
	TRACE_ARGS_ETC,
	[...]
};

struct tracecmd_msg_trace_args {
	be32		type;
	be32		argc;
};

That way the type will define the arguments being sent, and the
other side will know what is expected of the args, and even test to
make sure they make sense.

-- Steve
diff mbox series

Patch

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 5961355..6a21e66 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -331,6 +331,18 @@  int tracecmd_msg_collect_data(struct tracecmd_msg_handle *msg_handle, int ofd);
 bool tracecmd_msg_done(struct tracecmd_msg_handle *msg_handle);
 void tracecmd_msg_set_done(struct tracecmd_msg_handle *msg_handle);
 
+int tracecmd_msg_send_trace_req(struct tracecmd_msg_handle *msg_handle,
+				int argc, char **argv);
+int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
+				int *argc, char ***argv);
+
+int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
+				 int nr_cpus, int page_size,
+				 unsigned int *ports);
+int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
+				 int *nr_cpus, int *page_size,
+				 unsigned int **ports);
+
 /* --- Plugin handling --- */
 extern struct tep_plugin_option trace_ftrace_options[];
 
diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 51d0ac8..8ce1f98 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -16,6 +16,7 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
+#include <string.h>
 #include <unistd.h>
 #include <arpa/inet.h>
 #include <sys/types.h>
@@ -64,6 +65,17 @@  struct tracecmd_msg_rinit {
 	be32 cpus;
 } __attribute__((packed));
 
+struct tracecmd_msg_trace_req {
+	be32 flags;
+	be32 argc;
+} __attribute__((packed));
+
+struct tracecmd_msg_trace_resp {
+	be32 flags;
+	be32 cpus;
+	be32 page_size;
+} __attribute__((packed));
+
 struct tracecmd_msg_header {
 	be32	size;
 	be32	cmd;
@@ -76,7 +88,9 @@  struct tracecmd_msg_header {
 	C(RINIT,	2,	sizeof(struct tracecmd_msg_rinit)),	\
 	C(SEND_DATA,	3,	0),					\
 	C(FIN_DATA,	4,	0),					\
-	C(NOT_SUPP,	5,	0),
+	C(NOT_SUPP,	5,	0),					\
+	C(TRACE_REQ,	6,	sizeof(struct tracecmd_msg_trace_req)),	\
+	C(TRACE_RESP,	7,	sizeof(struct tracecmd_msg_trace_resp)),
 
 #undef C
 #define C(a,b,c)	MSG_##a = b
@@ -108,6 +122,8 @@  struct tracecmd_msg {
 	union {
 		struct tracecmd_msg_tinit	tinit;
 		struct tracecmd_msg_rinit	rinit;
+		struct tracecmd_msg_trace_req	trace_req;
+		struct tracecmd_msg_trace_resp	trace_resp;
 	};
 	union {
 		struct tracecmd_msg_opt		*opt;
@@ -723,3 +739,196 @@  error:
 	msg_free(&msg);
 	return ret;
 }
+
+static int make_trace_req(struct tracecmd_msg *msg, int argc, char **argv)
+{
+	size_t args_size = 0;
+	char *p;
+	int i;
+
+	for (i = 0; i < argc; i++)
+		args_size += strlen(argv[i]) + 1;
+
+	msg->hdr.size = htonl(ntohl(msg->hdr.size) + args_size);
+	msg->trace_req.argc = htonl(argc);
+	msg->buf = calloc(args_size, 1);
+	if (!msg->buf)
+		return -ENOMEM;
+
+	p = msg->buf;
+	for (i = 0; i < argc; i++)
+		p = stpcpy(p, argv[i]) + 1;
+
+	return 0;
+}
+
+int tracecmd_msg_send_trace_req(struct tracecmd_msg_handle *msg_handle,
+				int argc, char **argv)
+{
+	struct tracecmd_msg msg;
+	int ret;
+
+	tracecmd_msg_init(MSG_TRACE_REQ, &msg);
+	ret = make_trace_req(&msg, argc, argv);
+	if (ret < 0)
+		return ret;
+
+	return tracecmd_msg_send(msg_handle->fd, &msg);
+}
+
+ /*
+  * NOTE: On success, the returned `argv` should be freed with:
+  *     free(argv[0]);
+  *     free(argv);
+  */
+int tracecmd_msg_recv_trace_req(struct tracecmd_msg_handle *msg_handle,
+				int *argc, char ***argv)
+{
+	struct tracecmd_msg msg;
+	char *p, *buf_end, **args;
+	int i, ret, nr_args;
+	ssize_t buf_len;
+
+	ret = tracecmd_msg_recv(msg_handle->fd, &msg);
+	if (ret < 0)
+		return ret;
+
+	if (ntohl(msg.hdr.cmd) != MSG_TRACE_REQ) {
+		ret = -ENOTSUP;
+		goto out;
+	}
+
+	nr_args = ntohl(msg.trace_req.argc);
+	if (nr_args <= 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	buf_end = (char *)msg.buf + buf_len;
+	if (buf_len <= 0 && ((char *)msg.buf)[buf_len-1] != '\0') {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	args = calloc(nr_args, sizeof(*args));
+	if (!args) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	p = msg.buf;
+	for (i = 0; i < nr_args; i++) {
+		if (p >= buf_end) {
+			ret = -EINVAL;
+			goto out_args;
+		}
+		args[i] = p;
+		p = strchr(p, '\0');
+		if (!p) {
+			ret = -EINVAL;
+			goto out_args;
+		}
+		p++;
+	}
+
+	*argc = nr_args;
+	*argv = args;
+
+	/*
+	 * On success we're passing msg.buf to the caller through argv[0] so we
+	 * reset it here before calling msg_free().
+	 */
+	msg.buf = NULL;
+	msg_free(&msg);
+	return 0;
+
+out_args:
+	free(args);
+out:
+	error_operation(&msg);
+	if (ret == -EOPNOTSUPP)
+		handle_unexpected_msg(msg_handle, &msg);
+	msg_free(&msg);
+	return ret;
+}
+
+static int make_trace_resp(struct tracecmd_msg *msg,
+			   int page_size, int nr_cpus, unsigned int *ports)
+{
+	int ports_size = nr_cpus * sizeof(*msg->port_array);
+	int i;
+
+	msg->hdr.size = htonl(ntohl(msg->hdr.size) + ports_size);
+	msg->trace_resp.cpus = htonl(nr_cpus);
+	msg->trace_resp.page_size = htonl(page_size);
+
+	msg->port_array = malloc(ports_size);
+	if (!msg->port_array)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_cpus; i++)
+		msg->port_array[i] = htonl(ports[i]);
+
+	return 0;
+}
+
+int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
+				 int nr_cpus, int page_size,
+				 unsigned int *ports)
+{
+	struct tracecmd_msg msg;
+	int ret;
+
+	tracecmd_msg_init(MSG_TRACE_RESP, &msg);
+	ret = make_trace_resp(&msg, page_size, nr_cpus, ports);
+	if (ret < 0)
+		return ret;
+
+	return tracecmd_msg_send(msg_handle->fd, &msg);
+}
+
+int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
+				 int *nr_cpus, int *page_size,
+				 unsigned int **ports)
+{
+	struct tracecmd_msg msg;
+	ssize_t buf_len;
+	int i, ret;
+
+	ret = tracecmd_msg_recv(msg_handle->fd, &msg);
+	if (ret < 0)
+		return ret;
+
+	if (ntohl(msg.hdr.cmd) != MSG_TRACE_RESP) {
+		ret = -ENOTSUP;
+		goto out;
+	}
+
+	buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
+	if (buf_len <= 0 ||
+	    buf_len != sizeof(*msg.port_array) * ntohl(msg.trace_resp.cpus)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	*nr_cpus = ntohl(msg.trace_resp.cpus);
+	*page_size = ntohl(msg.trace_resp.page_size);
+	*ports = calloc(*nr_cpus, sizeof(**ports));
+	if (!*ports) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	for (i = 0; i < *nr_cpus; i++)
+		(*ports)[i] = ntohl(msg.port_array[i]);
+
+	msg_free(&msg);
+	return 0;
+
+out:
+	error_operation(&msg);
+	if (ret == -EOPNOTSUPP)
+		handle_unexpected_msg(msg_handle, &msg);
+	msg_free(&msg);
+	return ret;
+}