diff mbox series

[v2,09/10] trace-cmd: Use the new flow when creating output handler

Message ID 20211111150730.86323-1-tz.stoyanov@gmail.com (mailing list archive)
State Accepted
Commit 07283ab56f42514ae416cb75111c0756158a7022
Headers show
Series Refactor APIs for creating output handler | expand

Commit Message

Tzvetomir Stoyanov (VMware) Nov. 11, 2021, 3:07 p.m. UTC 
The trace-cmd commands, that create a new output handler to a trace
file, are converted to use the newly introduced trace-cmd APIs.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 tracecmd/trace-record.c  | 55 +++++++++++++++++++++++++++++++++++-----
 tracecmd/trace-restore.c | 32 +++++++++++++++++++++--
 2 files changed, 79 insertions(+), 8 deletions(-)

Comments

Steven Rostedt Nov. 24, 2021, 4:07 a.m. UTC | #1 
On Thu, 11 Nov 2021 17:07:30 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> @@ -4437,6 +4456,30 @@ static void write_guest_file(struct buffer_instance *instance)
>  	free(temp_files);
>  }
>  
> +static struct tracecmd_output *create_output(struct common_record_context *ctx)
> +{
> +	struct tracecmd_output *out;
> +	int fd;
> +
> +	fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);

I stopped at this patch because I really dislike the above.

Why don't we have:

	tracecmd_output_allocate(file);

and
	tracecmd_output_allocate_fd(fd);

Where tracecmd_output_allocate(file) does:

struct tracecmd_output *tracecmd_output_allocate(const char *file)
{
	int fd;

	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
	if (fd < 0)
		return NULL;
	return tracecmd_output_allocate_fd(fd);
}

?

Then we could remove a lot of these duplicate opens all over the place.

Although, I'm not sure I like the name allocate. It probably should be called:

tracecmd_output_create();

and we keep tracecmd_output_allocate() as is?

-- Steve


> +	if (fd < 0)
> +		return NULL;
> +
> +	out = tracecmd_output_allocate(fd);
> +	if (!out)
> +		goto error;
> +	if (tracecmd_output_write_headers(out, listed_events))
> +		goto error;
> +	return out;
> +error:
> +	if (out)
Tzvetomir Stoyanov (VMware) Nov. 26, 2021, 1:33 p.m. UTC | #2 
On Wed, Nov 24, 2021 at 6:08 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 11 Nov 2021 17:07:30 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > @@ -4437,6 +4456,30 @@ static void write_guest_file(struct buffer_instance *instance)
> >       free(temp_files);
> >  }
> >
> > +static struct tracecmd_output *create_output(struct common_record_context *ctx)
> > +{
> > +     struct tracecmd_output *out;
> > +     int fd;
> > +
> > +     fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
>
> I stopped at this patch because I really dislike the above.
>
> Why don't we have:
>
>         tracecmd_output_allocate(file);
>
> and
>         tracecmd_output_allocate_fd(fd);
>
> Where tracecmd_output_allocate(file) does:
>
> struct tracecmd_output *tracecmd_output_allocate(const char *file)
> {
>         int fd;
>
>         fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
>         if (fd < 0)
>                 return NULL;
>         return tracecmd_output_allocate_fd(fd);
> }
>
> ?
>
> Then we could remove a lot of these duplicate opens all over the place.
>
> Although, I'm not sure I like the name allocate. It probably should be called:
>
> tracecmd_output_create();
>
> and we keep tracecmd_output_allocate() as is?
>

 There is already such API, I'll replace that pattern with a call to this:
   struct tracecmd_output *tracecmd_create_init_file(const char *output_file)

> -- Steve
>
>
> > +     if (fd < 0)
> > +             return NULL;
> > +
> > +     out = tracecmd_output_allocate(fd);
> > +     if (!out)
> > +             goto error;
> > +     if (tracecmd_output_write_headers(out, listed_events))
> > +             goto error;
> > +     return out;
> > +error:
> > +     if (out)
diff mbox series

Patch

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 1767a6c6..15e07cf0 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -3689,6 +3689,25 @@  again:
 
 static void add_options(struct tracecmd_output *handle, struct common_record_context *ctx);
 
+static struct tracecmd_output *create_net_output(struct common_record_context *ctx,
+						 struct tracecmd_msg_handle *msg_handle)
+{
+	struct tracecmd_output *out;
+
+	out = tracecmd_output_allocate(-1);
+	if (!out)
+		return NULL;
+	if (tracecmd_output_set_msg(out, msg_handle))
+		goto error;
+	if (tracecmd_output_write_headers(out, listed_events))
+		goto error;
+
+	return out;
+error:
+	tracecmd_output_close(out);
+	return NULL;
+}
+
 static struct tracecmd_msg_handle *
 setup_connection(struct buffer_instance *instance, struct common_record_context *ctx)
 {
@@ -3700,7 +3719,7 @@  setup_connection(struct buffer_instance *instance, struct common_record_context
 
 	/* Now create the handle through this socket */
 	if (msg_handle->version == V3_PROTOCOL) {
-		network_handle = tracecmd_create_init_fd_msg(msg_handle, listed_events);
+		network_handle = create_net_output(ctx, msg_handle);
 		if (!network_handle)
 			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
@@ -3718,10 +3737,11 @@  setup_connection(struct buffer_instance *instance, struct common_record_context
 		if (ret)
 			goto error;
 	} else {
-		network_handle = tracecmd_create_init_fd_glob(msg_handle->fd,
-							      listed_events);
+		network_handle = tracecmd_output_allocate(msg_handle->fd);
 		if (!network_handle)
 			goto error;
+		if (tracecmd_output_write_headers(network_handle, listed_events))
+			goto error;
 		tracecmd_set_quiet(network_handle, quiet);
 	}
 
@@ -4067,8 +4087,7 @@  static void setup_agent(struct buffer_instance *instance,
 {
 	struct tracecmd_output *network_handle;
 
-	network_handle = tracecmd_create_init_fd_msg(instance->msg_handle,
-						     listed_events);
+	network_handle = create_net_output(ctx, instance->msg_handle);
 	add_options(network_handle, ctx);
 	tracecmd_write_cmdlines(network_handle);
 	tracecmd_write_cpus(network_handle, instance->cpu_count);
@@ -4437,6 +4456,30 @@  static void write_guest_file(struct buffer_instance *instance)
 	free(temp_files);
 }
 
+static struct tracecmd_output *create_output(struct common_record_context *ctx)
+{
+	struct tracecmd_output *out;
+	int fd;
+
+	fd = open(ctx->output, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
+		return NULL;
+
+	out = tracecmd_output_allocate(fd);
+	if (!out)
+		goto error;
+	if (tracecmd_output_write_headers(out, listed_events))
+		goto error;
+	return out;
+error:
+	if (out)
+		tracecmd_output_close(out);
+	else
+		close(fd);
+	unlink(ctx->output);
+	return NULL;
+}
+
 static void record_data(struct common_record_context *ctx)
 {
 	struct tracecmd_option **buffer_options;
@@ -4491,7 +4534,7 @@  static void record_data(struct common_record_context *ctx)
 				touch_file(temp_files[i]);
 		}
 
-		handle = tracecmd_create_init_file_glob(ctx->output, listed_events);
+		handle = create_output(ctx);
 		if (!handle)
 			die("Error creating output file");
 		tracecmd_set_quiet(handle, quiet);
diff --git a/tracecmd/trace-restore.c b/tracecmd/trace-restore.c
index 280a37f0..8d2fcae8 100644
--- a/tracecmd/trace-restore.c
+++ b/tracecmd/trace-restore.c
@@ -22,6 +22,35 @@ 
 
 #include "trace-local.h"
 
+static struct tracecmd_output *create_output(const char *file,
+					     const char *tracing_dir, const char *kallsyms)
+{
+	struct tracecmd_output *out;
+	int fd;
+
+	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
+	if (fd < 0)
+		return NULL;
+
+	out = tracecmd_output_allocate(fd);
+	if (!out)
+		goto error;
+	if (tracing_dir && tracecmd_output_set_trace_dir(out, tracing_dir))
+		goto error;
+	if (kallsyms && tracecmd_output_set_kallsyms(out, kallsyms))
+		goto error;
+	if (tracecmd_output_write_headers(out, NULL))
+		goto error;
+	return out;
+error:
+	if (out)
+		tracecmd_output_close(out);
+	else
+		close(fd);
+	unlink(file);
+	return NULL;
+}
+
 void trace_restore (int argc, char **argv)
 {
 	struct tracecmd_output *handle;
@@ -90,8 +119,7 @@  void trace_restore (int argc, char **argv)
 			usage(argv);
 		}
 
-		handle = tracecmd_create_init_file_override(output, tracing_dir,
-							    kallsyms);
+		handle = create_output(output, tracing_dir, kallsyms);
 		if (!handle)
 			die("Unabled to create output file %s", output);
 		if (tracecmd_write_cmdlines(handle) < 0)