| Message ID | 20210914131232.3964615-8-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd fixes and clean-ups | expand |
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; > } >
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; 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; + } + 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; }
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(-)