[2/2] trace-cmd: Fix broken listener and add error checks

Commit Message

Tzvetomir Stoyanov (VMware) Feb. 17, 2021, 4:23 a.m. UTC

The trace-cmd listener was broken by recent changes in writing saved
command lines in the trace file. There was no error checking in that
flow, so the trace-cmd listener indicates successful trace session,
even though the output trace file was broken.
Fixed the listener logic and added more error checks.

Signed-off-by: Tzvetomir Stoyanov (VMware) <[e-mail]>
---
 tracecmd/trace-listen.c | 22 ++++++++++++++--------
 tracecmd/trace-record.c | 31 ++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 13 deletions(-)

Comments

Steven Rostedt Feb. 17, 2021, 7:46 p.m. UTC | #1

> @@ -634,11 +638,12 @@ static int process_client(struct tracecmd_msg_handle *msg_handle,
>  	stop_msg_handle = msg_handle;
>  
>  	/* Now we are ready to start reading data from the client */
> -	if (msg_handle->version == V3_PROTOCOL)
> -		tracecmd_msg_collect_data(msg_handle, ofd);
> -	else
> +	if (msg_handle->version == V3_PROTOCOL) {
> +		ret = tracecmd_msg_collect_data(msg_handle, ofd);
> +	} else {
>  		collect_metadata_from_client(msg_handle, ofd);
> -
> +		ret = 0;
> +	}

We should have collect_metadata_from_client() return an error too, and keep
the if statement simple (no need for brackets);

	if (msg_handle->version == V3_PROTOCOL)
		ret = tracecmd_msg_collect_data(msg_handle, ofd);
	else
		ret = collect_metadata_from_client(msg_handle, ofd);

There's a few "pdie"s in that function that should be converted to error
returns.

-- Steve


>  	stop_msg_handle = NULL;
>  
>  	/* wait a little to let our readers finish reading */

Patch

diff --git a/tracecmd/trace-listen.c b/tracecmd/trace-listen.c
index 7798fe4f..ecad7656 100644
--- a/tracecmd/trace-listen.c
+++ b/tracecmd/trace-listen.c
@@ -597,8 +597,12 @@  static int put_together_file(int cpus, int ofd, const char *node,
 	}
 
 	if (write_options) {
-		tracecmd_write_cpus(handle, cpus);
-		tracecmd_write_options(handle);
+		ret = tracecmd_write_cpus(handle, cpus);
+		if (ret)
+			goto out;
+		ret = tracecmd_write_options(handle);
+		if (ret)
+			goto out;
 	}
 	ret = tracecmd_write_cpu_data(handle, cpus, temp_files);
 
@@ -634,11 +638,12 @@  static int process_client(struct tracecmd_msg_handle *msg_handle,
 	stop_msg_handle = msg_handle;
 
 	/* Now we are ready to start reading data from the client */
-	if (msg_handle->version == V3_PROTOCOL)
-		tracecmd_msg_collect_data(msg_handle, ofd);
-	else
+	if (msg_handle->version == V3_PROTOCOL) {
+		ret = tracecmd_msg_collect_data(msg_handle, ofd);
+	} else {
 		collect_metadata_from_client(msg_handle, ofd);
-
+		ret = 0;
+	}
 	stop_msg_handle = NULL;
 
 	/* wait a little to let our readers finish reading */
@@ -652,8 +657,9 @@  static int process_client(struct tracecmd_msg_handle *msg_handle,
 	/* wait a little to have the readers clean up */
 	sleep(1);
 
-	ret = put_together_file(cpus, ofd, node, port,
-				msg_handle->version < V3_PROTOCOL);
+	if (!ret)
+		ret = put_together_file(cpus, ofd, node, port,
+					msg_handle->version < V3_PROTOCOL);
 
 	destroy_all_readers(cpus, pid_array, node, port);
 
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 9396042d..f6131692 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3590,22 +3590,36 @@  static void add_options(struct tracecmd_output *handle, struct common_record_con
 static struct tracecmd_msg_handle *
 setup_connection(struct buffer_instance *instance, struct common_record_context *ctx)
 {
-	struct tracecmd_msg_handle *msg_handle;
-	struct tracecmd_output *network_handle;
+	struct tracecmd_msg_handle *msg_handle = NULL;
+	struct tracecmd_output *network_handle = NULL;
+	int ret;
 
 	msg_handle = setup_network(instance);
 
 	/* Now create the handle through this socket */
 	if (msg_handle->version == V3_PROTOCOL) {
 		network_handle = tracecmd_create_init_fd_msg(msg_handle, listed_events);
+		if (!network_handle)
+			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
 		add_options(network_handle, ctx);
-		tracecmd_write_cpus(network_handle, instance->cpu_count);
-		tracecmd_write_options(network_handle);
-		tracecmd_msg_finish_sending_data(msg_handle);
+		ret = tracecmd_write_cmdlines(network_handle);
+		if (ret)
+			goto error;
+		ret = tracecmd_write_cpus(network_handle, instance->cpu_count);
+		if (ret)
+			goto error;
+		ret = tracecmd_write_options(network_handle);
+		if (ret)
+			goto error;
+		ret = tracecmd_msg_finish_sending_data(msg_handle);
+		if (ret)
+			goto error;
 	} else {
 		network_handle = tracecmd_create_init_fd_glob(msg_handle->fd,
 							      listed_events);
+		if (!network_handle)
+			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
 	}
 
@@ -3613,6 +3627,13 @@  setup_connection(struct buffer_instance *instance, struct common_record_context
 
 	/* OK, we are all set, let'r rip! */
 	return msg_handle;
+
+error:
+	if (msg_handle)
+		tracecmd_msg_handle_close(msg_handle);
+	if (network_handle)
+		tracecmd_output_close(network_handle);
+	return NULL;
 }
 
 static void finish_network(struct tracecmd_msg_handle *msg_handle)