Linux-Trace-Devel Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/8] trace-cmd protocol fixes
@ 2019-02-04  7:08 Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 1/8] trace-cmd: Remove unused global variable Slavomir Kaslev
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

This patch set tries to make trace-cmd's protocol more robust to future
changes. New MSG_NOT_SUPP message is added for acknowledgement of unexpected
requests.

Slavomir Kaslev (8):
  trace-cmd: Remove unused global variable
  trace-cmd: Rename error_operation_for_server
  trace-cmd: Remove tracecmd_msg_handle/tracecmd_msg_server distinction
  trace-cmd: Check if connection is done when reading data in
    tracecmd_msg_read_data
  trace-cmd: Fix a memory leak in tracecmd_msg_send_init_data
  trace-cmd: Make tracecmd_msg_send_close return error code if any
  trace-cmd: Add tracecmd_msg_wait_close function
  trace-cmd: Acknowledge unexpected protocol messages

 include/trace-cmd/trace-cmd.h |  14 +--
 tracecmd/trace-listen.c       |   2 +-
 tracecmd/trace-msg.c          | 172 ++++++++++++++++++----------------
 tracecmd/trace-record.c       |   2 +-
 4 files changed, 96 insertions(+), 94 deletions(-)

-- 
2.19.1


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

* [PATCH 1/8] trace-cmd: Remove unused global variable
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 2/8] trace-cmd: Rename error_operation_for_server Slavomir Kaslev
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

No changes in behavior intended.

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

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 529ae2a..418a22d 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -130,8 +130,6 @@ struct tracecmd_msg {
 	};
 } __attribute__((packed));
 
-struct tracecmd_msg *errmsg;
-
 static int msg_write(int fd, struct tracecmd_msg *msg)
 {
 	int cmd = ntohl(msg->hdr.cmd);
-- 
2.19.1


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

* [PATCH 2/8] trace-cmd: Rename error_operation_for_server
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 1/8] trace-cmd: Remove unused global variable Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 3/8] trace-cmd: Remove tracecmd_msg_handle/tracecmd_msg_server distinction Slavomir Kaslev
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

No changes in behavior intended.

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

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 418a22d..ffebce7 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -345,6 +345,12 @@ void tracecmd_msg_set_done(struct tracecmd_msg_handle *msg_handle)
 	msg_server->done = true;
 }
 
+static void error_operation(struct tracecmd_msg *msg)
+{
+	warning("Message: cmd=%d size=%d\n",
+		ntohl(msg->hdr.cmd), ntohl(msg->hdr.size));
+}
+
 /*
  * A return value of 0 indicates time-out
  */
@@ -432,15 +438,6 @@ static bool process_option(struct tracecmd_msg_handle *msg_handle,
 	return false;
 }
 
-static void error_operation_for_server(struct tracecmd_msg *msg)
-{
-	u32 cmd;
-
-	cmd = ntohl(msg->hdr.cmd);
-
-	warning("Message: cmd=%d size=%d\n", cmd, ntohl(msg->hdr.size));
-}
-
 struct tracecmd_msg_handle *
 tracecmd_msg_handle_alloc(int fd, unsigned long flags)
 {
@@ -545,7 +542,7 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 	return pagesize;
 
 error:
-	error_operation_for_server(&msg);
+	error_operation(&msg);
 	return ret;
 }
 
@@ -670,7 +667,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 	return 0;
 
 error:
-	error_operation_for_server(&msg);
+	error_operation(&msg);
 	msg_free(&msg);
 	return ret;
 }
@@ -709,7 +706,7 @@ int tracecmd_msg_collect_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 	return 0;
 
 error:
-	error_operation_for_server(&msg);
+	error_operation(&msg);
 	msg_free(&msg);
 	return ret;
 }
-- 
2.19.1


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

* [PATCH 3/8] trace-cmd: Remove tracecmd_msg_handle/tracecmd_msg_server distinction
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 1/8] trace-cmd: Remove unused global variable Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 2/8] trace-cmd: Rename error_operation_for_server Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 4/8] trace-cmd: Check if connection is done when reading data in tracecmd_msg_read_data Slavomir Kaslev
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

The difference between tracecmd_msg_handle and tracecmd_msg_server is a single
bool and the tracecmd_msg_set_done/tracecmd_msg_done functions are also useful
on the client side in the context of the tracing VMs over vsockets work.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 include/trace-cmd/trace-cmd.h | 11 ++---------
 tracecmd/trace-listen.c       |  2 +-
 tracecmd/trace-msg.c          | 31 +++----------------------------
 tracecmd/trace-record.c       |  2 +-
 4 files changed, 7 insertions(+), 39 deletions(-)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 86b9b53..33f352b 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -293,16 +293,8 @@ void tracecmd_disable_all_tracing(int disable_tracer);
 void tracecmd_disable_tracing(void);
 void tracecmd_enable_tracing(void);
 
-enum tracecmd_msg_bits {
-	TRACECMD_MSG_BIT_CLIENT		= 0,
-	TRACECMD_MSG_BIT_SERVER		= 1,
-	TRACECMD_MSG_BIT_USE_TCP	= 2,
-};
-
 enum tracecmd_msg_flags {
-	TRACECMD_MSG_FL_CLIENT		= (1 << TRACECMD_MSG_BIT_CLIENT),
-	TRACECMD_MSG_FL_SERVER		= (1 << TRACECMD_MSG_BIT_SERVER),
-	TRACECMD_MSG_FL_USE_TCP		= (1 << TRACECMD_MSG_BIT_USE_TCP),
+	TRACECMD_MSG_FL_USE_TCP		= 1 << 0,
 };
 
 /* for both client and server */
@@ -311,6 +303,7 @@ struct tracecmd_msg_handle {
 	short			cpu_count;
 	short			version;	/* Current protocol version */
 	unsigned long		flags;
+	bool			done;
 };
 
 struct tracecmd_msg_handle *
diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 9b50147..8bd7bad 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -748,7 +748,7 @@ static int do_connection(int cfd, struct sockaddr_storage *peer_addr,
 	if (ret)
 		return ret;
 
-	msg_handle = tracecmd_msg_handle_alloc(cfd, TRACECMD_MSG_FL_SERVER);
+	msg_handle = tracecmd_msg_handle_alloc(cfd, 0);
 
 	s = getnameinfo((struct sockaddr *)peer_addr, peer_addr_len,
 			host, NI_MAXHOST,
diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index ffebce7..ef2a6d8 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -49,21 +49,6 @@ static inline void dprint(const char *fmt, ...)
 
 unsigned int page_size;
 
-struct tracecmd_msg_server {
-	struct tracecmd_msg_handle handle;
-	int			done;
-};
-
-static struct tracecmd_msg_server *
-make_server(struct tracecmd_msg_handle *msg_handle)
-{
-	if (!(msg_handle->flags & TRACECMD_MSG_FL_SERVER)) {
-		plog("Message handle not of type server\n");
-		return NULL;
-	}
-	return (struct tracecmd_msg_server *)msg_handle;
-}
-
 struct tracecmd_msg_opt {
 	be32 size;
 	be32 opt_cmd;
@@ -333,16 +318,12 @@ static int msg_wait_to = MSG_WAIT_MSEC;
 
 bool tracecmd_msg_done(struct tracecmd_msg_handle *msg_handle)
 {
-	struct tracecmd_msg_server *msg_server = make_server(msg_handle);
-
-	return (volatile int)msg_server->done;
+	return (volatile int)msg_handle->done;
 }
 
 void tracecmd_msg_set_done(struct tracecmd_msg_handle *msg_handle)
 {
-	struct tracecmd_msg_server *msg_server = make_server(msg_handle);
-
-	msg_server->done = true;
+	msg_handle->done = true;
 }
 
 static void error_operation(struct tracecmd_msg *msg)
@@ -442,14 +423,8 @@ struct tracecmd_msg_handle *
 tracecmd_msg_handle_alloc(int fd, unsigned long flags)
 {
 	struct tracecmd_msg_handle *handle;
-	int size;
-
-	if (flags == TRACECMD_MSG_FL_SERVER)
-		size = sizeof(struct tracecmd_msg_server);
-	else
-		size = sizeof(struct tracecmd_msg_handle);
 
-	handle = calloc(1, size);
+	handle = calloc(1, sizeof(struct tracecmd_msg_handle));
 	if (!handle)
 		return NULL;
 
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 3034a4b..8beefab 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -2883,7 +2883,7 @@ again:
 	if (msg_handle) {
 		msg_handle->fd = sfd;
 	} else {
-		msg_handle = tracecmd_msg_handle_alloc(sfd, TRACECMD_MSG_FL_CLIENT);
+		msg_handle = tracecmd_msg_handle_alloc(sfd, 0);
 		if (!msg_handle)
 			die("Failed to allocate message handle");
 
-- 
2.19.1


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

* [PATCH 4/8] trace-cmd: Check if connection is done when reading data in tracecmd_msg_read_data
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
                   ` (2 preceding siblings ...)
  2019-02-04  7:08 ` [PATCH 3/8] trace-cmd: Remove tracecmd_msg_handle/tracecmd_msg_server distinction Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 5/8] trace-cmd: Fix a memory leak in tracecmd_msg_send_init_data Slavomir Kaslev
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

Now that tracecmd_msg_done/tracecmd_msg_set_done can be used in both server and
client context, check if the connection is shutting down in tracecmd_msg_read_data.

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 tracecmd/trace-msg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index ef2a6d8..f7ce863 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -603,7 +603,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 	ssize_t s;
 	int ret;
 
-	for (;;) {
+	while (!tracecmd_msg_done(msg_handle)) {
 		ret = tracecmd_msg_recv_wait(msg_handle->fd, &msg);
 		if (ret < 0) {
 			if (ret == -ETIMEDOUT)
-- 
2.19.1


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

* [PATCH 5/8] trace-cmd: Fix a memory leak in tracecmd_msg_send_init_data
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
                   ` (3 preceding siblings ...)
  2019-02-04  7:08 ` [PATCH 4/8] trace-cmd: Check if connection is done when reading data in tracecmd_msg_read_data Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any Slavomir Kaslev
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

Fix tracecmd_msg_send_init_data leaking memory for the messages it receives.

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

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index f7ce863..b4b58d4 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -373,8 +373,7 @@ static int tracecmd_msg_wait_for_msg(int fd, struct tracecmd_msg *msg)
 int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 				unsigned int **client_ports)
 {
-	struct tracecmd_msg send_msg;
-	struct tracecmd_msg recv_msg;
+	struct tracecmd_msg msg;
 	int fd = msg_handle->fd;
 	unsigned int *ports;
 	int i, cpus;
@@ -382,30 +381,41 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 
 	*client_ports = NULL;
 
-	tracecmd_msg_init(MSG_TINIT, &send_msg);
-	ret = make_tinit(msg_handle, &send_msg);
+	tracecmd_msg_init(MSG_TINIT, &msg);
+	ret = make_tinit(msg_handle, &msg);
 	if (ret < 0)
-		return ret;
+		goto out;
 
-	ret = tracecmd_msg_send(fd, &send_msg);
+	ret = tracecmd_msg_send(fd, &msg);
 	if (ret < 0)
-		return ret;
+		goto out;
+
+	msg_free(&msg);
 
-	ret = tracecmd_msg_wait_for_msg(fd, &recv_msg);
+	ret = tracecmd_msg_wait_for_msg(fd, &msg);
 	if (ret < 0)
-		return ret;
+		goto out;
 
-	if (ntohl(recv_msg.hdr.cmd) != MSG_RINIT)
-		return -EINVAL;
+	if (ntohl(msg.hdr.cmd) != MSG_RINIT) {
+		ret = -EINVAL;
+		goto error;
+	}
 
-	cpus = ntohl(recv_msg.rinit.cpus);
+	cpus = ntohl(msg.rinit.cpus);
 	ports = malloc_or_die(sizeof(*ports) * cpus);
 	for (i = 0; i < cpus; i++)
-		ports[i] = ntohl(recv_msg.port_array[i]);
+		ports[i] = ntohl(msg.port_array[i]);
 
 	*client_ports = ports;
 
+	msg_free(&msg);
 	return 0;
+
+error:
+	error_operation(&msg);
+out:
+	msg_free(&msg);
+	return ret;
 }
 
 static bool process_option(struct tracecmd_msg_handle *msg_handle,
-- 
2.19.1


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

* [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
                   ` (4 preceding siblings ...)
  2019-02-04  7:08 ` [PATCH 5/8] trace-cmd: Fix a memory leak in tracecmd_msg_send_init_data Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-05 15:14   ` Steven Rostedt
  2019-02-04  7:08 ` [PATCH 7/8] trace-cmd: Add tracecmd_msg_wait_close function Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 8/8] trace-cmd: Acknowledge unexpected protocol messages Slavomir Kaslev
  7 siblings, 1 reply; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
---
 include/trace-cmd/trace-cmd.h | 2 +-
 tracecmd/trace-msg.c          | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 33f352b..0ab23f6 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -318,7 +318,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
 			       const char *buf, int size);
 int tracecmd_msg_finish_sending_data(struct tracecmd_msg_handle *msg_handle);
-void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
+int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
 
 /* for server */
 int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle);
diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index b4b58d4..c24424b 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -549,12 +549,12 @@ int tracecmd_msg_send_port_array(struct tracecmd_msg_handle *msg_handle,
 	return 0;
 }
 
-void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
+int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
 {
 	struct tracecmd_msg msg;
 
 	tracecmd_msg_init(MSG_CLOSE, &msg);
-	tracecmd_msg_send(msg_handle->fd, &msg);
+	return tracecmd_msg_send(msg_handle->fd, &msg);
 }
 
 int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
-- 
2.19.1


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

* [PATCH 7/8] trace-cmd: Add tracecmd_msg_wait_close function
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
                   ` (5 preceding siblings ...)
  2019-02-04  7:08 ` [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  2019-02-04  7:08 ` [PATCH 8/8] trace-cmd: Acknowledge unexpected protocol messages Slavomir Kaslev
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

Add tracecmd_msg_wait_close function that waits for MSG_CLOSE and logs any
invalid messages it receives.

Also switch tracecmd_msg_collect_data to use the new function while at it.

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

diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
index 0ab23f6..ca4452b 100644
--- a/include/trace-cmd/trace-cmd.h
+++ b/include/trace-cmd/trace-cmd.h
@@ -319,6 +319,7 @@ int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
 			       const char *buf, int size);
 int tracecmd_msg_finish_sending_data(struct tracecmd_msg_handle *msg_handle);
 int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
+int tracecmd_msg_wait_close(struct tracecmd_msg_handle *msg_handle);
 
 /* for server */
 int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle);
diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index c24424b..5079d43 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -659,39 +659,34 @@ error:
 
 int tracecmd_msg_collect_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 {
-	struct tracecmd_msg msg;
-	u32 cmd;
 	int ret;
 
 	ret = tracecmd_msg_read_data(msg_handle, ofd);
 	if (ret)
-		goto error;
+		return ret;
+
+	return tracecmd_msg_wait_close(msg_handle);
+}
 
-	/* check the finish message of the client */
+int tracecmd_msg_wait_close(struct tracecmd_msg_handle *msg_handle)
+{
+	struct tracecmd_msg msg;
+	int ret = -1;
+
+	memset(&msg, 0, sizeof(msg));
 	while (!tracecmd_msg_done(msg_handle)) {
 		ret = tracecmd_msg_recv(msg_handle->fd, &msg);
-		if (ret < 0) {
-			warning("reading client");
-			return ret;
-		}
-
-		cmd = ntohl(msg.hdr.cmd);
-		if (cmd == MSG_CLOSE)
-			/* Finish this connection */
-			break;
-		else {
-			warning("Not accept the message %d", ntohl(msg.hdr.cmd));
-			ret = -EINVAL;
+		if (ret < 0)
 			goto error;
-		}
 
+		if (ntohl(msg.hdr.cmd) == MSG_CLOSE)
+			return 0;
+
+		error_operation(&msg);
 		msg_free(&msg);
 	}
 
-	return 0;
-
 error:
-	error_operation(&msg);
 	msg_free(&msg);
 	return ret;
 }
-- 
2.19.1


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

* [PATCH 8/8] trace-cmd: Acknowledge unexpected protocol messages
  2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
                   ` (6 preceding siblings ...)
  2019-02-04  7:08 ` [PATCH 7/8] trace-cmd: Add tracecmd_msg_wait_close function Slavomir Kaslev
@ 2019-02-04  7:08 ` Slavomir Kaslev
  7 siblings, 0 replies; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-04  7:08 UTC (permalink / raw)
  To: linux-trace-devel; +Cc: rostedt, slavomir.kaslev, tstoyanov, ykaradzhov

Send MSG_NOT_SUPP message back on unexpected incoming messages. This allows us
to add new commands in the future and be able to detect and handle if we're
talking with an older version of trace-cmd.

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

diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
index 5079d43..51d0ac8 100644
--- a/tracecmd/trace-msg.c
+++ b/tracecmd/trace-msg.c
@@ -75,7 +75,8 @@ struct tracecmd_msg_header {
 	C(TINIT,	1,	sizeof(struct tracecmd_msg_tinit)),	\
 	C(RINIT,	2,	sizeof(struct tracecmd_msg_rinit)),	\
 	C(SEND_DATA,	3,	0),					\
-	C(FIN_DATA,	4,	0),
+	C(FIN_DATA,	4,	0),					\
+	C(NOT_SUPP,	5,	0),
 
 #undef C
 #define C(a,b,c)	MSG_##a = b
@@ -370,6 +371,25 @@ static int tracecmd_msg_wait_for_msg(int fd, struct tracecmd_msg *msg)
 	return 0;
 }
 
+static int tracecmd_msg_send_notsupp(struct tracecmd_msg_handle *msg_handle)
+{
+	struct tracecmd_msg msg;
+
+	tracecmd_msg_init(MSG_NOT_SUPP, &msg);
+	return tracecmd_msg_send(msg_handle->fd, &msg);
+}
+
+static int handle_unexpected_msg(struct tracecmd_msg_handle *msg_handle,
+				 struct tracecmd_msg *msg)
+{
+	/* Don't send MSG_NOT_SUPP back if we just received one */
+	if (ntohl(msg->hdr.cmd) == MSG_NOT_SUPP)
+		return 0;
+
+	return tracecmd_msg_send_notsupp(msg_handle);
+
+}
+
 int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 				unsigned int **client_ports)
 {
@@ -397,7 +417,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 		goto out;
 
 	if (ntohl(msg.hdr.cmd) != MSG_RINIT) {
-		ret = -EINVAL;
+		ret = -EOPNOTSUPP;
 		goto error;
 	}
 
@@ -413,6 +433,8 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
 
 error:
 	error_operation(&msg);
+	if (ret == -EOPNOTSUPP)
+		handle_unexpected_msg(msg_handle, &msg);
 out:
 	msg_free(&msg);
 	return ret;
@@ -461,7 +483,6 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 	int ret;
 	int offset = 0;
 	u32 size;
-	u32 cmd;
 
 	ret = tracecmd_msg_recv_wait(msg_handle->fd, &msg);
 	if (ret < 0) {
@@ -470,9 +491,8 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 		return ret;
 	}
 
-	cmd = ntohl(msg.hdr.cmd);
-	if (cmd != MSG_TINIT) {
-		ret = -EINVAL;
+	if (ntohl(msg.hdr.cmd) != MSG_TINIT) {
+		ret = -EOPNOTSUPP;
 		goto error;
 	}
 
@@ -524,10 +544,14 @@ int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle)
 		}
 	}
 
+	msg_free(&msg);
 	return pagesize;
 
 error:
 	error_operation(&msg);
+	if (ret == -EOPNOTSUPP)
+		handle_unexpected_msg(msg_handle, &msg);
+	msg_free(&msg);
 	return ret;
 }
 
@@ -627,8 +651,12 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 		if (cmd == MSG_FIN_DATA) {
 			/* Finish receiving data */
 			break;
-		} else if (cmd != MSG_SEND_DATA)
-			goto error;
+		} else if (cmd != MSG_SEND_DATA) {
+			ret = handle_unexpected_msg(msg_handle, &msg);
+			if (ret < 0)
+				goto error;
+			goto next;
+		}
 
 		n = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
 		t = n;
@@ -646,6 +674,7 @@ int tracecmd_msg_read_data(struct tracecmd_msg_handle *msg_handle, int ofd)
 			s = n - t;
 		}
 
+next:
 		msg_free(&msg);
 	}
 
@@ -683,6 +712,10 @@ int tracecmd_msg_wait_close(struct tracecmd_msg_handle *msg_handle)
 			return 0;
 
 		error_operation(&msg);
+		ret = handle_unexpected_msg(msg_handle, &msg);
+		if (ret < 0)
+			goto error;
+
 		msg_free(&msg);
 	}
 
-- 
2.19.1


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

* Re: [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any
  2019-02-04  7:08 ` [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any Slavomir Kaslev
@ 2019-02-05 15:14   ` Steven Rostedt
  2019-02-07 12:52     ` Slavomir Kaslev
  2019-02-08 19:34     ` Steven Rostedt
  0 siblings, 2 replies; 13+ messages in thread
From: Steven Rostedt @ 2019-02-05 15:14 UTC (permalink / raw)
  To: Slavomir Kaslev; +Cc: linux-trace-devel, slavomir.kaslev, tstoyanov, ykaradzhov

On Mon,  4 Feb 2019 09:08:53 +0200
Slavomir Kaslev <kaslevs@vmware.com> wrote:

I accepted your patches up to here with some slight modifications to
the subjects and change logs. Note, when referencing function names,
its mostly desirable to add a "()" to the end of them to make it stand
out that they are functions. Like tracecmd_msg_send_close().

But, this patch needs a change log to explain why this function should
return an error code. Is something going to rely on it in the future?

-- Steve



> Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
> ---
>  include/trace-cmd/trace-cmd.h | 2 +-
>  tracecmd/trace-msg.c          | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index 33f352b..0ab23f6 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -318,7 +318,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
>  int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
>  			       const char *buf, int size);
>  int tracecmd_msg_finish_sending_data(struct tracecmd_msg_handle *msg_handle);
> -void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
> +int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
>  
>  /* for server */
>  int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle);
> diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
> index b4b58d4..c24424b 100644
> --- a/tracecmd/trace-msg.c
> +++ b/tracecmd/trace-msg.c
> @@ -549,12 +549,12 @@ int tracecmd_msg_send_port_array(struct tracecmd_msg_handle *msg_handle,
>  	return 0;
>  }
>  
> -void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
> +int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
>  {
>  	struct tracecmd_msg msg;
>  
>  	tracecmd_msg_init(MSG_CLOSE, &msg);
> -	tracecmd_msg_send(msg_handle->fd, &msg);
> +	return tracecmd_msg_send(msg_handle->fd, &msg);
>  }
>  
>  int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,


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

* Re: [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any
  2019-02-05 15:14   ` Steven Rostedt
@ 2019-02-07 12:52     ` Slavomir Kaslev
  2019-02-07 14:41       ` Steven Rostedt
  2019-02-08 19:34     ` Steven Rostedt
  1 sibling, 1 reply; 13+ messages in thread
From: Slavomir Kaslev @ 2019-02-07 12:52 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel, Tzvetomir Stoyanov, Yordan Karadzhov

On Tue, Feb 5, 2019 at 5:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon,  4 Feb 2019 09:08:53 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> I accepted your patches up to here with some slight modifications to
> the subjects and change logs. Note, when referencing function names,
> its mostly desirable to add a "()" to the end of them to make it stand
> out that they are functions. Like tracecmd_msg_send_close().
>
> But, this patch needs a change log to explain why this function should
> return an error code. Is something going to rely on it in the future?

Apologies for the empty commit message on this one, somehow it slipped
me before sending.

The intention for this patch was simply stylistic, as with close()
which returns error code but it's seldom checked.

Feel free to drop it if you find that unnecessary. Patch 7 and 8 don't
depend on it. Should I resend them?

-- Slavi


>
> -- Steve
>
>
>
> > Signed-off-by: Slavomir Kaslev <kaslevs@vmware.com>
> > ---
> >  include/trace-cmd/trace-cmd.h | 2 +-
> >  tracecmd/trace-msg.c          | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> > index 33f352b..0ab23f6 100644
> > --- a/include/trace-cmd/trace-cmd.h
> > +++ b/include/trace-cmd/trace-cmd.h
> > @@ -318,7 +318,7 @@ int tracecmd_msg_send_init_data(struct tracecmd_msg_handle *msg_handle,
> >  int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
> >                              const char *buf, int size);
> >  int tracecmd_msg_finish_sending_data(struct tracecmd_msg_handle *msg_handle);
> > -void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
> > +int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle);
> >
> >  /* for server */
> >  int tracecmd_msg_initial_setting(struct tracecmd_msg_handle *msg_handle);
> > diff --git a/tracecmd/trace-msg.c b/tracecmd/trace-msg.c
> > index b4b58d4..c24424b 100644
> > --- a/tracecmd/trace-msg.c
> > +++ b/tracecmd/trace-msg.c
> > @@ -549,12 +549,12 @@ int tracecmd_msg_send_port_array(struct tracecmd_msg_handle *msg_handle,
> >       return 0;
> >  }
> >
> > -void tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
> > +int tracecmd_msg_send_close_msg(struct tracecmd_msg_handle *msg_handle)
> >  {
> >       struct tracecmd_msg msg;
> >
> >       tracecmd_msg_init(MSG_CLOSE, &msg);
> > -     tracecmd_msg_send(msg_handle->fd, &msg);
> > +     return tracecmd_msg_send(msg_handle->fd, &msg);
> >  }
> >
> >  int tracecmd_msg_data_send(struct tracecmd_msg_handle *msg_handle,
>

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

* Re: [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any
  2019-02-07 12:52     ` Slavomir Kaslev
@ 2019-02-07 14:41       ` Steven Rostedt
  0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2019-02-07 14:41 UTC (permalink / raw)
  To: Slavomir Kaslev; +Cc: linux-trace-devel, Tzvetomir Stoyanov, Yordan Karadzhov

On Thu, 7 Feb 2019 12:52:03 +0000
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> On Tue, Feb 5, 2019 at 5:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Mon,  4 Feb 2019 09:08:53 +0200
> > Slavomir Kaslev <kaslevs@vmware.com> wrote:
> >
> > I accepted your patches up to here with some slight modifications to
> > the subjects and change logs. Note, when referencing function names,
> > its mostly desirable to add a "()" to the end of them to make it stand
> > out that they are functions. Like tracecmd_msg_send_close().
> >
> > But, this patch needs a change log to explain why this function should
> > return an error code. Is something going to rely on it in the future?  
> 
> Apologies for the empty commit message on this one, somehow it slipped
> me before sending.
> 
> The intention for this patch was simply stylistic, as with close()
> which returns error code but it's seldom checked.
> 
> Feel free to drop it if you find that unnecessary. Patch 7 and 8 don't
> depend on it. Should I resend them?
> 

I'll drop this patch and apply the other two (after looking a bit more
at them). No need to resend.

Thanks,

-- Steve

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

* Re: [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any
  2019-02-05 15:14   ` Steven Rostedt
  2019-02-07 12:52     ` Slavomir Kaslev
@ 2019-02-08 19:34     ` Steven Rostedt
  1 sibling, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2019-02-08 19:34 UTC (permalink / raw)
  To: Slavomir Kaslev; +Cc: linux-trace-devel, slavomir.kaslev, tstoyanov, ykaradzhov

On Tue, 5 Feb 2019 10:14:49 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon,  4 Feb 2019 09:08:53 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
> 
> I accepted your patches up to here with some slight modifications to
> the subjects and change logs. Note, when referencing function names,
> its mostly desirable to add a "()" to the end of them to make it stand
> out that they are functions. Like tracecmd_msg_send_close().
> 
> But, this patch needs a change log to explain why this function should
> return an error code. Is something going to rely on it in the future?
> 

I think I'll take this patch anyway (and add a change log to it). Since
it can fail, it would be good to return the error.

-- Steve

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

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04  7:08 [PATCH 0/8] trace-cmd protocol fixes Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 1/8] trace-cmd: Remove unused global variable Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 2/8] trace-cmd: Rename error_operation_for_server Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 3/8] trace-cmd: Remove tracecmd_msg_handle/tracecmd_msg_server distinction Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 4/8] trace-cmd: Check if connection is done when reading data in tracecmd_msg_read_data Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 5/8] trace-cmd: Fix a memory leak in tracecmd_msg_send_init_data Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 6/8] trace-cmd: Make tracecmd_msg_send_close return error code if any Slavomir Kaslev
2019-02-05 15:14   ` Steven Rostedt
2019-02-07 12:52     ` Slavomir Kaslev
2019-02-07 14:41       ` Steven Rostedt
2019-02-08 19:34     ` Steven Rostedt
2019-02-04  7:08 ` [PATCH 7/8] trace-cmd: Add tracecmd_msg_wait_close function Slavomir Kaslev
2019-02-04  7:08 ` [PATCH 8/8] trace-cmd: Acknowledge unexpected protocol messages Slavomir Kaslev

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 linux-trace-devel@archiver.kernel.org
	public-inbox-index linux-trace-devel


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