linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v3 19/21] trace-cmd library: Refactor APIs for creating output handler
Date: Mon, 4 Oct 2021 18:37:44 -0400	[thread overview]
Message-ID: <20211004183744.03dd6cd7@gandalf.local.home> (raw)
In-Reply-To: <20210914131232.3964615-20-tz.stoyanov@gmail.com>

On Tue, 14 Sep 2021 16:12:30 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> In the trace-cmd library there are various APIs for allocating and
> initializing output handler to a trace file. The existing APIs are use
> case oriented, with a lot of parameters. Extending them for new use
> cases, adding more input parameters, will make the library more complex
> and not easy to use.
> Almost all use case oriented APIs for output handler creation are
> removed and replaced with new flow, which is easier to be extended with
> new creation parameters.
> 
> Removed APIs:
>  tracecmd_create_init_fd_msg()
>  tracecmd_create_init_file_glob()
>  tracecmd_create_init_fd_glob()
>  tracecmd_create_init_file_override()
> 
> New APIs:
>  tracecmd_output_allocate()
>  tracecmd_output_set_msg()
>  tracecmd_output_set_trace_dir()
>  tracecmd_output_set_kallsyms()
>  tracecmd_output_set_from_input()
>  tracecmd_output_write_init()
>  tracecmd_output_write_headers()

So this patch is pretty much impossible to review and know if anything
broke, because it does many changes, and not just one. It's basically a
rewrite of a lot of code, and I can't tell what repercussions it may have.

That is, it needs to be broken up into at least two patches, maybe more.

> 
> The new tracecmd_output_allocate() API allocates memory and performs
> minimal initialization of an output handler to a trace file. No data
> is written in the file.
> The tracecmd_output_set_...() APIs can be used to set various
> parameters to the newly allocated output handler, that affect the way
> the data is written into the file.
> When the output handler is configured for the desired use case, the
> tracecmd_output_write_init() is used to start writing to the file, it
> writes initial magic bytes.
> The tracecmd_output_write_headers() API is used to write the initial
> headers into the file.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  .../include/private/trace-cmd-private.h       |  25 +-
>  lib/trace-cmd/trace-output.c                  | 406 ++++++++++++------
>  tracecmd/trace-record.c                       |  55 ++-
>  tracecmd/trace-restore.c                      |  32 +-
>  4 files changed, 356 insertions(+), 162 deletions(-)
> 
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index c58b09e9..167f3804 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -102,7 +102,8 @@ static inline int tracecmd_host_bigendian(void)
>  /* --- Opening and Reading the trace.dat file --- */
>  
>  enum tracecmd_file_states {
> -	TRACECMD_FILE_INIT = 1,
> +	TRACECMD_FILE_ALLOCATED = 0,
> +	TRACECMD_FILE_INIT,
>  	TRACECMD_FILE_HEADERS,
>  	TRACECMD_FILE_FTRACE_EVENTS,
>  	TRACECMD_FILE_ALL_EVENTS,
> @@ -268,20 +269,20 @@ struct tracecmd_event_list {
>  struct tracecmd_option;
>  struct tracecmd_msg_handle;
>  
> +struct tracecmd_output *tracecmd_output_allocate(int fd);
> +int tracecmd_output_set_msg(struct tracecmd_output *handler,
> +			    struct tracecmd_msg_handle *msg_handle);
> +int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir);
> +int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms);
> +int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle);
> +int tracecmd_output_write_init(struct tracecmd_output *handler);
> +int tracecmd_output_write_headers(struct tracecmd_output *handler,
> +				  struct tracecmd_event_list *list);
> +
>  struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, int cpus);
> -struct tracecmd_output *
> -tracecmd_create_init_file_glob(const char *output_file,
> -			       struct tracecmd_event_list *list);
>  struct tracecmd_output *tracecmd_create_init_fd(int fd);
> -struct tracecmd_output *
> -tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list);
> -struct tracecmd_output *
> -tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
> -			    struct tracecmd_event_list *list);
> +
>  struct tracecmd_output *tracecmd_create_init_file(const char *output_file);
> -struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
> -							   const char *tracing_dir,
> -							   const char *kallsyms);
>  struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle,
>  					    unsigned short id, int size,
>  					    const void *data);
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index fe1d1aaa..20116d99 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -31,11 +31,6 @@
>  typedef unsigned long long	tsize_t;
>  typedef long long		stsize_t;
>  
> -static struct tracecmd_event_list all_event_list = {
> -	.next = NULL,
> -	.glob = "all"
> -};
> -
>  struct tracecmd_option {
>  	unsigned short	id;
>  	int		size;
> @@ -54,11 +49,14 @@ struct tracecmd_output {
>  	int			cpus;
>  	struct tep_handle	*pevent;
>  	char			*tracing_dir;
> +	char			*kallsyms;
> +	struct tracecmd_event_list *events;
>  	int			nr_options;
>  	bool			quiet;
>  	unsigned long		file_state;
>  	unsigned long		file_version;
>  	size_t			options_start;
> +	bool			big_endian;
>  
>  	struct list_head	options;
>  	struct tracecmd_msg_handle *msg_handle;
> @@ -682,7 +680,7 @@ static int read_event_files(struct tracecmd_output *handle,
>  			break;
>  	}
>  	/* all events are listed, use a global glob */
> -	if (list)
> +	if (!event_list || list)
>  		event_list = &all_events;
>  
>  	systems = create_event_list(handle, event_list);
> @@ -752,8 +750,7 @@ err:
>  		tracecmd_warning("can't set kptr_restrict");
>  }
>  
> -static int read_proc_kallsyms(struct tracecmd_output *handle,
> -			      const char *kallsyms)
> +static int read_proc_kallsyms(struct tracecmd_output *handle)
>  {
>  	unsigned int size, check_size, endian4;
>  	const char *path = "/proc/kallsyms";
> @@ -766,8 +763,8 @@ static int read_proc_kallsyms(struct tracecmd_output *handle,
>  		return -1;
>  	}
>  
> -	if (kallsyms)
> -		path = kallsyms;
> +	if (handle->kallsyms)
> +		path = handle->kallsyms;
>  
>  	ret = stat(path, &st);
>  	if (ret < 0) {
> @@ -883,137 +880,247 @@ out_free:
>  	return ret;
>  }
>  
> -static int select_file_version(struct tracecmd_output *handle,
> -				struct tracecmd_input *ihandle)
> -{
> -	if (ihandle)
> -		handle->file_version = tracecmd_get_in_file_version(ihandle);
> -	else
> -		handle->file_version = FILE_VERSION;
> -
> -	return 0;
> -}
> -
> -static struct tracecmd_output *
> -create_file_fd(int fd, struct tracecmd_input *ihandle,
> -	       const char *tracing_dir,
> -	       const char *kallsyms,
> -	       struct tracecmd_event_list *list,
> -	       struct tracecmd_msg_handle *msg_handle)

Instead of just removing this, add tarcecmd_output_alloct() that this uses
first.

> +/**
> + * tracecmd_output_allocate - allocate new output handler to a trace file
> + * @handle: file descriptor to an empty file, it can be -1 if the handler
> + *	    will not write to a file
> + *
> + * This API only allocates a handler and performs minimal initialization.
> + * No data is written in the file.
> + *
> + * Returns pointer to a newly allocated file descriptor, that can be used
> + * with other library APIs. In case of an error, NULL is returned. The returned
> + * handler must be freed with tracecmd_output_close() or tracecmd_output_free()
> + */
> +struct tracecmd_output *tracecmd_output_allocate(int fd)
>  {
>  	struct tracecmd_output *handle;
> -	struct tep_handle *pevent;
> -	char buf[BUFSIZ];
> -	int endian4;
>  
> -	handle = malloc(sizeof(*handle));
> +	handle = calloc(1, sizeof(*handle));
>  	if (!handle)
>  		return NULL;
> -	memset(handle, 0, sizeof(*handle));
> -
> -	list_head_init(&handle->options);
>  
>  	handle->fd = fd;
> -	if (tracing_dir) {
> -		handle->tracing_dir = strdup(tracing_dir);
> -		if (!handle->tracing_dir)
> -			goto out_free;
> -	}
>  
> -	handle->msg_handle = msg_handle;
> +	handle->file_version = FILE_VERSION;
>  
> -	if (select_file_version(handle, ihandle))
> -		goto out_free;
> +	handle->page_size = getpagesize();
>  
> -	buf[0] = 23;
> -	buf[1] = 8;
> -	buf[2] = 68;
> -	memcpy(buf + 3, "tracing", 7);
> +	if (tracecmd_host_bigendian())
> +		handle->big_endian = true;
> +	else
> +		handle->big_endian = false;

By the way, the above is the same as:

	handle->big_endian = tracecmd_host_bigendian();

>  
> -	if (do_write_check(handle, buf, 10))
> -		goto out_free;
> +	list_head_init(&handle->options);
>  
> -	sprintf(buf, "%lu", handle->file_version);
> -	if (do_write_check(handle, buf, strlen(buf) + 1))
> -		goto out_free;
> +	handle->file_state = TRACECMD_FILE_ALLOCATED;
>  
> -	/* get endian and page size */
> -	if (ihandle) {
> -		pevent = tracecmd_get_tep(ihandle);
> -		/* Use the pevent of the ihandle for later writes */
> -		handle->pevent = tracecmd_get_tep(ihandle);
> -		tep_ref(pevent);
> -		if (tep_is_file_bigendian(pevent))
> -			buf[0] = 1;
> -		else
> -			buf[0] = 0;
> -		handle->page_size = tracecmd_page_size(ihandle);
> -	} else {
> -		if (tracecmd_host_bigendian())
> -			buf[0] = 1;
> -		else
> -			buf[0] = 0;
> -		handle->page_size = getpagesize();
> -	}
> +	return handle;
> +}
>  
> -	if (do_write_check(handle, buf, 1))
> -		goto out_free;
> +/**
> + * tracecmd_output_set_msg - associated an output file handler with network message handler
> + * @handle: output handler to a trace file.
> + * @msg_handle: network handler, allocated by tracecmd_msg_handle_alloc()
> + *
> + * This API associates an output file handler with a network stream. All subsequent API calls
> + * with this output file handler will send data over the network using the @msg_handle, instead
> + * of writing to a file.
> + * This API must be called after the handler file version is set and before
> + * tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_msg(struct tracecmd_output *handler, struct tracecmd_msg_handle *msg_handle)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	/* save size of long (this may not be what the kernel is) */
> -	buf[0] = sizeof(long);
> -	if (do_write_check(handle, buf, 1))
> -		goto out_free;
> +	handler->msg_handle = msg_handle;
>  
> -	endian4 = convert_endian_4(handle, handle->page_size);
> -	if (do_write_check(handle, &endian4, 4))
> -		goto out_free;
> -	handle->file_state = TRACECMD_FILE_INIT;
> +	return 0;
> +}
>  
> -	if (ihandle)
> -		return handle;
> +/**
> + * tracecmd_output_set_trace_dir - Set a custom tracing dir, instead of system default
> + * @handle: output handler to a trace file.
> + * @tracing_dir: full path to a directory with tracing files
> + *
> + * This API associates an output file handler with a custom tracing directory, to be used when
> + * creating the trace file instead of the system default tracing directory.
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_trace_dir(struct tracecmd_output *handler, const char *tracing_dir)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	if (read_header_files(handle))
> -		goto out_free;
> +	free(handler->tracing_dir);
> +	if (tracing_dir) {
> +		handler->tracing_dir = strdup(tracing_dir);
> +		if (!handler->tracing_dir)
> +			return -1;
> +	} else
> +		handler->tracing_dir = NULL;
>  
> -	if (read_ftrace_files(handle))
> -		goto out_free;
> +	return 0;
> +}
>  
> -	if (read_event_files(handle, list))
> -		goto out_free;
> +/**
> + * tracecmd_output_set_kallsyms - Set a custom kernel symbols file, instead of system default
> + * @handle: output handler to a trace file.
> + * @tracing_dir: full path to a file with kernel symbols
> + *
> + * This API associates an output file handler with a custom kernel symbols file, to be used when
> + * creating the trace file instead of the system default kernel symbols file.
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_kallsyms(struct tracecmd_output *handler, const char *kallsyms)
> +{
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	if (read_proc_kallsyms(handle, kallsyms))
> -		goto out_free;
> +	free(handler->kallsyms);
> +	if (kallsyms) {
> +		handler->kallsyms = strdup(kallsyms);
> +		if (!handler->kallsyms)
> +			return -1;
> +	} else
> +		handler->kallsyms = NULL;
>  
> -	if (read_ftrace_printk(handle))
> -		goto out_free;
> +	return 0;
> +}
>  
> -	return handle;
> +/**
> + * tracecmd_output_set_from_input - Inherit parameters from an existing trace file
> + * @handle: output handler to a trace file.
> + * @ihandle: input handler to an existing trace file.
> + *
> + * This API copies parameters from input handler @ihandle, associated with an existing trace file,
> + * to the output handler @handle, associated with file that is going to be created.
> + * These parameters are copied:
> + *  - tep handler
> + *  - page size
> + *  - file endian
> + *  - file version
> + *  - file compression protocol
> + * This API must be called before tracecmd_output_write_init().
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_set_from_input(struct tracecmd_output *handler, struct tracecmd_input *ihandle)
> +{
> +	if (!handler || !ihandle || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> - out_free:
> -	tracecmd_output_close(handle);
> -	return NULL;
> +	/* get endian, page size, file version and compression */
> +	/* Use the pevent of the ihandle for later writes */
> +	handler->pevent = tracecmd_get_tep(ihandle);
> +	tep_ref(handler->pevent);
> +	handler->page_size = tracecmd_page_size(ihandle);
> +	handler->file_version = tracecmd_get_in_file_version(ihandle);
> +	if (tep_is_file_bigendian(handler->pevent))
> +		handler->big_endian = true;
> +	else
> +		handler->big_endian = false;
> +
> +
> +	return 0;
>  }
>  
> -static struct tracecmd_output *create_file(const char *output_file,
> -					   struct tracecmd_input *ihandle,
> -					   const char *tracing_dir,
> -					   const char *kallsyms,
> -					   struct tracecmd_event_list *list)
> +/**
> + * tracecmd_output_write_init - Write the initial magics in the trace file
> + * @handle: output handler to a trace file.
> + *
> + * This API must be called after all tracecmd_output_set_...() APIs and before writing anything
> + * to the trace file. This initial information is written in the file:
> + *  - initial file magic bytes
> + *  - file version
> + *  - data endian
> + *  - long size
> + *  - page size
> + *  - compression header
> + *
> + * Returns 0 on success, or -1 if the output file handler is not allocated or not in expected state.
> + */
> +int tracecmd_output_write_init(struct tracecmd_output *handler)
>  {
> -	struct tracecmd_output *handle;
> -	int fd;
> +	char buf[BUFSIZ];
> +	int endian4;
>  
> -	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> -	if (fd < 0)
> -		return NULL;
> +	if (!handler || handler->file_state != TRACECMD_FILE_ALLOCATED)
> +		return -1;
>  
> -	handle = create_file_fd(fd, ihandle, tracing_dir, kallsyms, list, NULL);
> -	if (!handle) {
> -		close(fd);
> -		unlink(output_file);
> -	}
> +	buf[0] = 23;
> +	buf[1] = 8;
> +	buf[2] = 68;
> +	memcpy(buf + 3, "tracing", 7);
>  
> -	return handle;
> +	if (do_write_check(handler, buf, 10))
> +		return -1;
> +
> +	sprintf(buf, "%lu", handler->file_version);
> +	if (do_write_check(handler, buf, strlen(buf) + 1))
> +		return -1;
> +
> +	if (handler->big_endian)
> +		buf[0] = 1;
> +	else
> +		buf[0] = 0;
> +	if (do_write_check(handler, buf, 1))
> +		return -1;
> +
> +	/* save size of long (this may not be what the kernel is) */
> +	buf[0] = sizeof(long);
> +	if (do_write_check(handler, buf, 1))
> +		return -1;
> +
> +	endian4 = convert_endian_4(handler, handler->page_size);
> +	if (do_write_check(handler, &endian4, 4))
> +		return -1;
> +	handler->file_state = TRACECMD_FILE_INIT;
> +	return 0;
> +}
> +
> +/**
> + * tracecmd_output_write_headers - Write the trace file headers
> + * @handle: output handler to a trace file.
> + * @list: desired events that will be included in the trace file.
> + *	  It can be NULL for all available events
> + *
> + * These headers are written in the file:
> + *  - header files from the tracing directory
> + *  - ftrace events from the tracing directory
> + *  - event file from the tracing directory - all or only the one from @list
> + *  - kernel symbols from the tracing directory
> + *  - kernel printk strings from the tracing directory
> + *
> + * Returns 0 on success, or -1 in case of an error.
> + */
> +int tracecmd_output_write_headers(struct tracecmd_output *handler,
> +				  struct tracecmd_event_list *list)
> +{
> +	if (!handler || handler->file_state < TRACECMD_FILE_ALLOCATED)
> +		return -1;
> +
> +	/* Write init data, if not written yet */
> +	if (handler->file_state < TRACECMD_FILE_INIT && tracecmd_output_write_init(handler))
> +		return -1;
> +	if (read_header_files(handler))
> +		return -1;
> +	if (read_ftrace_files(handler))
> +		return -1;
> +	if (read_event_files(handler, list))
> +		return -1;
> +	if (read_proc_kallsyms(handler))
> +		return -1;
> +	if (read_ftrace_printk(handler))
> +		return -1;
> +	return 0;
>  }
>  
>  /**
> @@ -1267,11 +1374,19 @@ struct tracecmd_output *tracecmd_create_file_latency(const char *output_file, in
>  {
>  	struct tracecmd_output *handle;
>  	char *path;
> +	int fd;
>  
> -	handle = create_file(output_file, NULL, NULL, NULL, &all_event_list);
> -	if (!handle)
> +	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
>  		return NULL;
>  
> +	handle = tracecmd_output_allocate(fd);
> +	if (!handle)
> +		goto out_free;
> +	if (tracecmd_output_write_init(handle))
> +		goto out_free;
> +	if (tracecmd_output_write_headers(handle, NULL))
> +		goto out_free;
>  	/*
>  	 * Save the command lines;
>  	 */
> @@ -1565,39 +1680,38 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
>  
>  struct tracecmd_output *tracecmd_create_init_fd(int fd)
>  {
> -	return create_file_fd(fd, NULL, NULL, NULL, &all_event_list, NULL);
> -}
> -
> -struct tracecmd_output *
> -tracecmd_create_init_fd_msg(struct tracecmd_msg_handle *msg_handle,
> -			    struct tracecmd_event_list *list)
> -{
> -	return create_file_fd(msg_handle->fd, NULL, NULL, NULL, list, msg_handle);
> -}
> +	struct tracecmd_output *out;
>  
> -struct tracecmd_output *
> -tracecmd_create_init_fd_glob(int fd, struct tracecmd_event_list *list)
> -{
> -	return create_file_fd(fd, NULL, NULL, NULL, list, NULL);
> -}
> -
> -struct tracecmd_output *
> -tracecmd_create_init_file_glob(const char *output_file,
> -			       struct tracecmd_event_list *list)
> -{
> -	return create_file(output_file, NULL, NULL, NULL, list);
> +	out = tracecmd_output_allocate(fd);
> +	if (!out)
> +		return NULL;
> +	if (tracecmd_output_write_init(out))
> +		goto error;
> +	if (tracecmd_output_write_headers(out, NULL))
> +		goto error;
> +
> +	return out;
> +error:
> +	tracecmd_output_close(out);
> +	return NULL;
>  }
>  
>  struct tracecmd_output *tracecmd_create_init_file(const char *output_file)
>  {
> -	return create_file(output_file, NULL, NULL, NULL, &all_event_list);
> -}
> +	struct tracecmd_output *handle;
> +	int fd;
>  
> -struct tracecmd_output *tracecmd_create_init_file_override(const char *output_file,
> -							   const char *tracing_dir,
> -							   const char *kallsyms)
> -{
> -	return create_file(output_file, NULL, tracing_dir, kallsyms, &all_event_list);
> +	fd = open(output_file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
> +		return NULL;
> +	handle = tracecmd_create_init_fd(fd);
> +	if (!handle) {
> +		close(fd);
> +		unlink(output_file);
> +		return NULL;
> +	}
> +
> +	return handle;
>  }
>  
>  /**
> @@ -1613,11 +1727,19 @@ struct tracecmd_output *tracecmd_copy(struct tracecmd_input *ihandle,
>  				      const char *file)
>  {
>  	struct tracecmd_output *handle;
> +	int fd;
>  
> -	handle = create_file(file, ihandle, NULL, NULL, &all_event_list);
> -	if (!handle)
> +	fd = open(file, O_RDWR | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
> +	if (fd < 0)
>  		return NULL;
>  
> +	handle = tracecmd_output_allocate(fd);
> +	if (!handle)
> +		goto out_free;
> +	if (tracecmd_output_set_from_input(handle, ihandle))
> +		goto out_free;
> +	tracecmd_output_write_init(handle);
> +
>  	if (tracecmd_copy_headers(ihandle, handle->fd, 0, 0) < 0)
>  		goto out_free;
>  
> 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);

So basically tracecmd_create_init_fd_msg() could had been converted first
to do what create_net_output() does above, and then we can replace them.


>  		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);

Same here.

>  		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);

And here.

Basically, if you can introduce one API at a time, and have the old API use
the new one (maybe slightly modified), then it is possible to review this.
But as this patch is, I'll have to basically do as much work as you did to
create this patch for me to know if it is correct, and that's not the job of
a reviewer.

-- Steve


>  		if (!handle)
>  			die("Unabled to create output file %s", output);
>  		if (tracecmd_write_cmdlines(handle) < 0)


  reply	other threads:[~2021-10-04 22:37 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 13:12 [PATCH v3 00/21] trace-cmd fixes and clean-ups Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 01/21] trace-cmd library: Read option id with correct endian Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 02/21] trace-cmd report: Fix typos in error messages Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 03/21] trace-cmd library: Fix version string memory leak Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 04/21] trace-cmd library: Fixed a memory leak on input handler close Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 05/21] trace-cmd library: Do not pass guests list to a buffer instance Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 06/21] trace-cmd library: Set long size to the input tep handler when it is read from the file Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 07/21] trace-cmd library: Do not use local variables when reading CPU stat option Tzvetomir Stoyanov (VMware)
2021-10-04 15:35   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 08/21] trace-cmd library: Reuse within the library the function that checks file state Tzvetomir Stoyanov (VMware)
2021-10-04 15:43   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 09/21] trace-cmd library: Fix possible memory leak in read_ftrace_files() Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 10/21] trace-cmd library: Fix possible memory leak in read_event_files() Tzvetomir Stoyanov (VMware)
2021-10-04 15:47   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 11/21] trace-cmd library: Fix possible memory leak in read_proc_kallsyms() Tzvetomir Stoyanov (VMware)
2021-10-04 15:55   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 12/21] trace-cmd library: Fix possible memory leak in read_ftrace_printk() Tzvetomir Stoyanov (VMware)
2021-10-04 18:43   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 13/21] trace-cmd library: Fix possible memory leak in read_and_parse_cmdlines() Tzvetomir Stoyanov (VMware)
2021-10-04 18:44   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 14/21] trace-cmd library: Set input handler default values in allocation function Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 15/21] trace-cmd library: Track maximum CPUs count in input handler Tzvetomir Stoyanov (VMware)
2021-10-04 18:48   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 16/21] trace-cmd report: Close input file handlers on exit Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 17/21] trace-cmd report: Do not print empty buffer name Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 18/21] trace-cmd library: Set correct CPU to the record, retrieved with tracecmd_peek_data Tzvetomir Stoyanov (VMware)
2021-09-14 13:12 ` [PATCH v3 19/21] trace-cmd library: Refactor APIs for creating output handler Tzvetomir Stoyanov (VMware)
2021-10-04 22:37   ` Steven Rostedt [this message]
2021-09-14 13:12 ` [PATCH v3 20/21] trace-cmd library: Refactor the logic for writing trace data in the file Tzvetomir Stoyanov (VMware)
2021-10-05  0:51   ` Steven Rostedt
2021-09-14 13:12 ` [PATCH v3 21/21] trace-cmd report: Init the top trace instance earlier Tzvetomir Stoyanov (VMware)
2021-10-05  1:00 ` [PATCH v3 00/21] trace-cmd fixes and clean-ups Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211004183744.03dd6cd7@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=tz.stoyanov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).