On Sat, Jan 15, 2022 at 2:53 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 10 Dec 2021 12:54:27 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > .../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;
> > }
>
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|