On Tue, 14 Sep 2021 16:12:18 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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;
> }
>
|