Re: [PATCH v3 07/21] trace-cmd library: Do not use local variables when reading CPU stat option

[Steven Rostedt <rostedt@goodmis.org>]

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

> Using a local variable to read all CPUSTAT options assumes that all of
> them are in a single option section. While this is true for the current
> trace file version format, this assumption limits the design of a more
> flexible format with multiple options sections. Use input handler context
> instead of the local variable.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-input.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 3d7f1c48..d020cfae 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -138,6 +138,7 @@ struct tracecmd_input {
>  
>  	struct host_trace_info	host;
>  	double			ts2secs;
> +	unsigned int		cpustats_size;
>  	char *			cpustats;

The above places cpustats_size (4 bytes) between ts2secs (8 bytes) and
cpustats (8 bytes) on 64 bit machines. Thus, it creates a hole of 4 bytes
of padding.

Best to try to place 4 bytes integers in pairs, where they always take up 8
bytes.

Above this, there's:

	// 8 bytes
	unsigned long long	trace_id;

	// pair 1
	int			fd;
	int			long_size;

	// pair 2
	int			page_size;
	int			page_map_size;

	// pair 3
	int			cpus;
	int			ref;

	// pair 4
	int			nr_buffers;	/* buffer instances */
	bool			use_trace_clock;

	// pair 5
	bool			read_page;
	bool			use_pipe;

	// uneven set of 4 bytes
	int			file_version;

	// 8 bytes
	struct cpu_data 	*cpu_data;

You can move the 4 byte field after file_version to fill in that hole.

>  	char *			uname;
>  	char *			version;
> @@ -2658,7 +2659,6 @@ static int handle_options(struct tracecmd_input *handle)
>  	unsigned short option;
>  	unsigned int size;
>  	char *cpustats = NULL;
> -	unsigned int cpustats_size = 0;
>  	struct input_buffer_instance *buffer;
>  	struct hook_list *hook;
>  	char *buf;
> @@ -2738,12 +2738,16 @@ static int handle_options(struct tracecmd_input *handle)
>  			break;
>  		case TRACECMD_OPTION_CPUSTAT:
>  			buf[size-1] = '\n';
> -			cpustats = realloc(cpustats, cpustats_size + size + 1);
> -			if (!cpustats)
> -				return -ENOMEM;
> -			memcpy(cpustats + cpustats_size, buf, size);
> -			cpustats_size += size;
> -			cpustats[cpustats_size] = 0;
> +			cpustats = realloc(handle->cpustats,
> +					   handle->cpustats_size + size + 1);
> +			if (!cpustats) {
> +				ret = -ENOMEM;
> +				return ret;
> +			}

Changing:

			if (!cpustats)
				return -ENOMEM;

to
			if (!cpustats) {
				ret = -ENOMEM;
				return ret;
			}

Doesn't make sense, as you are just adding a useless use of a local
variable.

-- Steve


> +			memcpy(cpustats + handle->cpustats_size, buf, size);
> +			handle->cpustats_size += size;
> +			cpustats[handle->cpustats_size] = 0;
> +			handle->cpustats = cpustats;
>  			break;
>  		case TRACECMD_OPTION_BUFFER:
>  			/* A buffer instance is saved at the end of the file */
> @@ -2813,8 +2817,6 @@ static int handle_options(struct tracecmd_input *handle)
>  
>  	}
>  
> -	handle->cpustats = cpustats;
> -
>  	return 0;
>  }
>