| Message ID | 20211210105448.97850-5-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Trace file version 7 - sections | expand |
On Fri, 10 Dec 2021 12:54:27 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > In the trace file metadata there are various dynamic strings. Collecting > all these strings in a dedicated section in the file simplifies parsing > of the metadata. The string section is added in trace files version 7, > at the end of the file. Does it have to be at the end of the file? > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > .../include/private/trace-cmd-private.h | 2 + > lib/trace-cmd/trace-output.c | 63 +++++++++++++++++++ > tracecmd/trace-record.c | 1 + > 3 files changed, 66 insertions(+) > > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h > index ed25d879..ed8fbf11 100644 > --- a/lib/trace-cmd/include/private/trace-cmd-private.h > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h > @@ -138,6 +138,7 @@ enum { > TRACECMD_OPTION_TIME_SHIFT, > TRACECMD_OPTION_GUEST, > TRACECMD_OPTION_TSC2NSEC, > + TRACECMD_OPTION_STRINGS, > }; > > enum { > @@ -301,6 +302,7 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle); > int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus); > int tracecmd_write_cmdlines(struct tracecmd_output *handle); > int tracecmd_write_options(struct tracecmd_output *handle); > +int tracecmd_write_meta_strings(struct tracecmd_output *handle); > int tracecmd_append_options(struct tracecmd_output *handle); > void tracecmd_output_close(struct tracecmd_output *handle); > void tracecmd_output_free(struct tracecmd_output *handle); > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c > index 4d165ac2..ed505db6 100644 > --- a/lib/trace-cmd/trace-output.c > +++ b/lib/trace-cmd/trace-output.c > @@ -62,6 +62,8 @@ struct tracecmd_output { > bool quiet; > unsigned long file_state; > unsigned long file_version; > + unsigned long strings_p; > + unsigned long strings_offs; Can you add a comment to what the above are to represent? Especially out of context, it's hard to know how this is suppose to work. > size_t options_start; > bool big_endian; > > @@ -69,6 +71,8 @@ struct tracecmd_output { > struct list_head buffers; > struct tracecmd_msg_handle *msg_handle; > char *trace_clock; > + char *strings; > + Remove the extra space. > }; > > struct list_event { > @@ -85,6 +89,8 @@ struct list_event_system { > > #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) > > +static int save_string_section(struct tracecmd_output *handle); > + I won't ask you to fix it, but it's best not to introduce a static function that is not used, and if need be, just combine the patches. Again, without use cases, it's hard to know if this is doing what you say it is doing. > static stsize_t > do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size) > { > @@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle, > return tep_read_number(handle->pevent, &val, 8); > } > > +static long add_string(struct tracecmd_output *handle, const char *string) > +{ > + int size = strlen(string) + 1; > + int pos = handle->strings_p; > + char *strings; > + > + strings = realloc(handle->strings, pos + size); > + if (!strings) > + return -1; > + handle->strings = strings; > + memcpy(handle->strings + pos, string, size); > + handle->strings_p += size; > + > + return handle->strings_offs + pos; What is the above suppose to be returning? What is strings_off? > +} > + > /** > * tracecmd_set_quiet - Set if to print output to the screen > * @quiet: If non zero, print no output to the screen > @@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle) > free(option); > } > > + free(handle->strings); > free(handle->trace_clock); > free(handle); > } > @@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle) > if (!handle) > return; > > + if (handle->file_version >= FILE_VERSION_SECTIONS) { Shouldn't the above be if (HAS_SECTIONS(handle)) ? -- Steve > + /* write strings section */ > + save_string_section(handle); > + } > + > if (handle->fd >= 0) { > close(handle->fd); > handle->fd = -1; > @@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set) > return ret; > } > > +static int save_string_section(struct tracecmd_output *handle) > +{ > + if (!handle->strings || !handle->strings_p) > + return 0; > + > + if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) { > + tracecmd_warning("Cannot write strings, unexpected state 0x%X", > + handle->file_state); > + return -1; > + } > + > + if (do_write_check(handle, handle->strings, handle->strings_p)) > + goto error; > + > + handle->strings_offs += handle->strings_p; > + free(handle->strings); > + handle->strings = NULL; > + handle->strings_p = 0; > + handle->file_state = TRACECMD_OPTION_STRINGS; > + return 0; > + > +error: > + return -1; > +} > + > + > static int read_header_files(struct tracecmd_output *handle) > { > tsize_t size, check_size, endian8; > @@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle) > return 0; > } > > +int tracecmd_write_meta_strings(struct tracecmd_output *handle) > +{ > + if (!HAS_SECTIONS(handle)) > + return 0; > + > + return save_string_section(handle); > +} > + > + > int tracecmd_append_options(struct tracecmd_output *handle) > { > struct tracecmd_option *options; > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index 7b2b59bb..f599610e 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance, > tracecmd_write_cmdlines(network_handle); > tracecmd_write_cpus(network_handle, instance->cpu_count); > tracecmd_write_options(network_handle); > + tracecmd_write_meta_strings(network_handle); > tracecmd_msg_finish_sending_data(instance->msg_handle); > instance->network_handle = network_handle; > }
On Sat, Jan 15, 2022 at 2:53 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 10 Dec 2021 12:54:27 +0200 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > In the trace file metadata there are various dynamic strings. Collecting > > all these strings in a dedicated section in the file simplifies parsing > > of the metadata. The string section is added in trace files version 7, > > at the end of the file. > > Does it have to be at the end of the file? The v7 file can have multiple string sections. The last string section must be at the end of the file, as it contains meta-data strings from the other sections. > > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- > > .../include/private/trace-cmd-private.h | 2 + > > lib/trace-cmd/trace-output.c | 63 +++++++++++++++++++ > > tracecmd/trace-record.c | 1 + > > 3 files changed, 66 insertions(+) > > > > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h > > index ed25d879..ed8fbf11 100644 > > --- a/lib/trace-cmd/include/private/trace-cmd-private.h > > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h > > @@ -138,6 +138,7 @@ enum { > > TRACECMD_OPTION_TIME_SHIFT, > > TRACECMD_OPTION_GUEST, > > TRACECMD_OPTION_TSC2NSEC, > > + TRACECMD_OPTION_STRINGS, > > }; > > > > enum { > > @@ -301,6 +302,7 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle); > > int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus); > > int tracecmd_write_cmdlines(struct tracecmd_output *handle); > > int tracecmd_write_options(struct tracecmd_output *handle); > > +int tracecmd_write_meta_strings(struct tracecmd_output *handle); > > int tracecmd_append_options(struct tracecmd_output *handle); > > void tracecmd_output_close(struct tracecmd_output *handle); > > void tracecmd_output_free(struct tracecmd_output *handle); > > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c > > index 4d165ac2..ed505db6 100644 > > --- a/lib/trace-cmd/trace-output.c > > +++ b/lib/trace-cmd/trace-output.c > > @@ -62,6 +62,8 @@ struct tracecmd_output { > > bool quiet; > > unsigned long file_state; > > unsigned long file_version; > > + unsigned long strings_p; > > + unsigned long strings_offs; > > Can you add a comment to what the above are to represent? > > Especially out of context, it's hard to know how this is suppose to > work. > > > size_t options_start; > > bool big_endian; > > > > @@ -69,6 +71,8 @@ struct tracecmd_output { > > struct list_head buffers; > > struct tracecmd_msg_handle *msg_handle; > > char *trace_clock; > > + char *strings; > > + > > Remove the extra space. > > > }; > > > > struct list_event { > > @@ -85,6 +89,8 @@ struct list_event_system { > > > > #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) > > > > +static int save_string_section(struct tracecmd_output *handle); > > + > > I won't ask you to fix it, but it's best not to introduce a static > function that is not used, and if need be, just combine the patches. > > Again, without use cases, it's hard to know if this is doing what you > say it is doing. > > > static stsize_t > > do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size) > > { > > @@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle, > > return tep_read_number(handle->pevent, &val, 8); > > } > > > > +static long add_string(struct tracecmd_output *handle, const char *string) > > +{ > > + int size = strlen(string) + 1; > > + int pos = handle->strings_p; > > + char *strings; > > + > > + strings = realloc(handle->strings, pos + size); > > + if (!strings) > > + return -1; > > + handle->strings = strings; > > + memcpy(handle->strings + pos, string, size); > > + handle->strings_p += size; > > + > > + return handle->strings_offs + pos; > > What is the above suppose to be returning? What is strings_off? > I'll add comments on strings_off and strings_p in the next version, but the main idea is: This function returns the virtual offset of the added string into the string section. There can be multiple string sections in the file, but the string offset is continuous and does not depend on the number of string sections and their position in the file. > > +} > > + > > /** > > * tracecmd_set_quiet - Set if to print output to the screen > > * @quiet: If non zero, print no output to the screen > > @@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle) > > free(option); > > } > > > > + free(handle->strings); > > free(handle->trace_clock); > > free(handle); > > } > > @@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle) > > if (!handle) > > return; > > > > + if (handle->file_version >= FILE_VERSION_SECTIONS) { > > Shouldn't the above be if (HAS_SECTIONS(handle)) ? > > -- Steve > > > + /* write strings section */ > > + save_string_section(handle); > > + } > > + > > if (handle->fd >= 0) { > > close(handle->fd); > > handle->fd = -1; > > @@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set) > > return ret; > > } > > > > +static int save_string_section(struct tracecmd_output *handle) > > +{ > > + if (!handle->strings || !handle->strings_p) > > + return 0; > > + > > + if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) { > > + tracecmd_warning("Cannot write strings, unexpected state 0x%X", > > + handle->file_state); > > + return -1; > > + } > > + > > + if (do_write_check(handle, handle->strings, handle->strings_p)) > > + goto error; > > + > > + handle->strings_offs += handle->strings_p; > > + free(handle->strings); > > + handle->strings = NULL; > > + handle->strings_p = 0; > > + handle->file_state = TRACECMD_OPTION_STRINGS; > > + return 0; > > + > > +error: > > + return -1; > > +} > > + > > + > > static int read_header_files(struct tracecmd_output *handle) > > { > > tsize_t size, check_size, endian8; > > @@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle) > > return 0; > > } > > > > +int tracecmd_write_meta_strings(struct tracecmd_output *handle) > > +{ > > + if (!HAS_SECTIONS(handle)) > > + return 0; > > + > > + return save_string_section(handle); > > +} > > + > > + > > int tracecmd_append_options(struct tracecmd_output *handle) > > { > > struct tracecmd_option *options; > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index 7b2b59bb..f599610e 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance, > > tracecmd_write_cmdlines(network_handle); > > tracecmd_write_cpus(network_handle, instance->cpu_count); > > tracecmd_write_options(network_handle); > > + tracecmd_write_meta_strings(network_handle); > > tracecmd_msg_finish_sending_data(instance->msg_handle); > > instance->network_handle = network_handle; > > } >
On Mon, 17 Jan 2022 11:32:10 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > Does it have to be at the end of the file? > > The v7 file can have multiple string sections. The last string section > must be at the end of the file, as it contains meta-data strings from > the other sections. Right, but it's not a requirement. It's more of what just happens. My concern is that it reads as a requirement not something that just happens to be an implementation side effect. -- Steve
On Mon, Jan 17, 2022 at 4:40 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 17 Jan 2022 11:32:10 +0200 > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > > > Does it have to be at the end of the file? > > > > The v7 file can have multiple string sections. The last string section > > must be at the end of the file, as it contains meta-data strings from > > the other sections. > > Right, but it's not a requirement. It's more of what just happens. My > concern is that it reads as a requirement not something that just > happens to be an implementation side effect. > Yes, it is not a requirement - as there is no strict order, any section can be anywhere in the file. > -- Steve
diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h index ed25d879..ed8fbf11 100644 --- a/lib/trace-cmd/include/private/trace-cmd-private.h +++ b/lib/trace-cmd/include/private/trace-cmd-private.h @@ -138,6 +138,7 @@ enum { TRACECMD_OPTION_TIME_SHIFT, TRACECMD_OPTION_GUEST, TRACECMD_OPTION_TSC2NSEC, + TRACECMD_OPTION_STRINGS, }; enum { @@ -301,6 +302,7 @@ int tracecmd_write_buffer_info(struct tracecmd_output *handle); int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus); int tracecmd_write_cmdlines(struct tracecmd_output *handle); int tracecmd_write_options(struct tracecmd_output *handle); +int tracecmd_write_meta_strings(struct tracecmd_output *handle); int tracecmd_append_options(struct tracecmd_output *handle); void tracecmd_output_close(struct tracecmd_output *handle); void tracecmd_output_free(struct tracecmd_output *handle); diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c index 4d165ac2..ed505db6 100644 --- a/lib/trace-cmd/trace-output.c +++ b/lib/trace-cmd/trace-output.c @@ -62,6 +62,8 @@ struct tracecmd_output { bool quiet; unsigned long file_state; unsigned long file_version; + unsigned long strings_p; + unsigned long strings_offs; size_t options_start; bool big_endian; @@ -69,6 +71,8 @@ struct tracecmd_output { struct list_head buffers; struct tracecmd_msg_handle *msg_handle; char *trace_clock; + char *strings; + }; struct list_event { @@ -85,6 +89,8 @@ struct list_event_system { #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) +static int save_string_section(struct tracecmd_output *handle); + static stsize_t do_write_check(struct tracecmd_output *handle, const void *data, tsize_t size) { @@ -127,6 +133,22 @@ static unsigned long long convert_endian_8(struct tracecmd_output *handle, return tep_read_number(handle->pevent, &val, 8); } +static long add_string(struct tracecmd_output *handle, const char *string) +{ + int size = strlen(string) + 1; + int pos = handle->strings_p; + char *strings; + + strings = realloc(handle->strings, pos + size); + if (!strings) + return -1; + handle->strings = strings; + memcpy(handle->strings + pos, string, size); + handle->strings_p += size; + + return handle->strings_offs + pos; +} + /** * tracecmd_set_quiet - Set if to print output to the screen * @quiet: If non zero, print no output to the screen @@ -185,6 +207,7 @@ void tracecmd_output_free(struct tracecmd_output *handle) free(option); } + free(handle->strings); free(handle->trace_clock); free(handle); } @@ -194,6 +217,11 @@ void tracecmd_output_close(struct tracecmd_output *handle) if (!handle) return; + if (handle->file_version >= FILE_VERSION_SECTIONS) { + /* write strings section */ + save_string_section(handle); + } + if (handle->fd >= 0) { close(handle->fd); handle->fd = -1; @@ -332,6 +360,32 @@ int tracecmd_ftrace_enable(int set) return ret; } +static int save_string_section(struct tracecmd_output *handle) +{ + if (!handle->strings || !handle->strings_p) + return 0; + + if (!check_out_state(handle, TRACECMD_OPTION_STRINGS)) { + tracecmd_warning("Cannot write strings, unexpected state 0x%X", + handle->file_state); + return -1; + } + + if (do_write_check(handle, handle->strings, handle->strings_p)) + goto error; + + handle->strings_offs += handle->strings_p; + free(handle->strings); + handle->strings = NULL; + handle->strings_p = 0; + handle->file_state = TRACECMD_OPTION_STRINGS; + return 0; + +error: + return -1; +} + + static int read_header_files(struct tracecmd_output *handle) { tsize_t size, check_size, endian8; @@ -1328,6 +1382,15 @@ int tracecmd_write_options(struct tracecmd_output *handle) return 0; } +int tracecmd_write_meta_strings(struct tracecmd_output *handle) +{ + if (!HAS_SECTIONS(handle)) + return 0; + + return save_string_section(handle); +} + + int tracecmd_append_options(struct tracecmd_output *handle) { struct tracecmd_option *options; diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index 7b2b59bb..f599610e 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -4093,6 +4093,7 @@ static void setup_agent(struct buffer_instance *instance, tracecmd_write_cmdlines(network_handle); tracecmd_write_cpus(network_handle, instance->cpu_count); tracecmd_write_options(network_handle); + tracecmd_write_meta_strings(network_handle); tracecmd_msg_finish_sending_data(instance->msg_handle); instance->network_handle = network_handle; }
In the trace file metadata there are various dynamic strings. Collecting all these strings in a dedicated section in the file simplifies parsing of the metadata. The string section is added in trace files version 7, at the end of the file. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- .../include/private/trace-cmd-private.h | 2 + lib/trace-cmd/trace-output.c | 63 +++++++++++++++++++ tracecmd/trace-record.c | 1 + 3 files changed, 66 insertions(+)