| Message ID | 20211111150900.86585-1-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Trace file version 7 - sections | expand |
On Thu, 11 Nov 2021 17:09:00 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > Introduce headers before each file section, in trace file version 7. The > section header has the following format: > <2 bytes>, header ID > <string>, null terminated ASCII string, description of the header > <2 bytes>, section flags: > 1: the section is compressed > <4 bytes>, size of the section > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > include/trace-cmd/trace-cmd.h | 5 ++ > .../include/private/trace-cmd-private.h | 1 + > lib/trace-cmd/include/trace-cmd-local.h | 5 ++ > lib/trace-cmd/trace-output.c | 68 +++++++++++++++++++ > 4 files changed, 79 insertions(+) > > diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h > index 7fea4e01..5d71e8ba 100644 > --- a/include/trace-cmd/trace-cmd.h > +++ b/include/trace-cmd/trace-cmd.h > @@ -15,6 +15,11 @@ enum tracecmd_open_flags { > TRACECMD_FL_LOAD_NO_PLUGINS = 1 << 0, /* Do not load plugins */ > TRACECMD_FL_LOAD_NO_SYSTEM_PLUGINS = 1 << 1, /* Do not load system plugins */ > }; > + > +enum tracecmd_section_flags { > + TRACECMD_SEC_FL_COMPRESS = 1 << 0, /* the section is compressed */ > +}; > + > struct tracecmd_input *tracecmd_open_head(const char *file, int flags); > struct tracecmd_input *tracecmd_open(const char *file, int flags); > struct tracecmd_input *tracecmd_open_fd(int fd, int flags); > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h > index eaa902a3..5b253511 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_MAX, > }; > > enum { > diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h > index d65ff599..d3760483 100644 > --- a/lib/trace-cmd/include/trace-cmd-local.h > +++ b/lib/trace-cmd/include/trace-cmd-local.h > @@ -37,6 +37,11 @@ struct data_file_write { > bool check_file_state(unsigned long file_version, int current_state, int new_state); > bool check_out_state(struct tracecmd_output *handle, int new_state); > > +unsigned long long > +out_write_section_header(struct tracecmd_output *handle, unsigned short header_id, > + char *description, enum tracecmd_section_flags flags, bool option); Unless you expect only a single flag to ever be passed in, flags cannot be of type enum. Because two enums or'd together is not a valid enum. -- Steve > +int out_update_section_header(struct tracecmd_output *handle, unsigned long long offset); > + > struct cpu_data_source { > int fd; > int size; > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c > index e8c788ad..a8c1305d 100644 > --- a/lib/trace-cmd/trace-output.c > +++ b/lib/trace-cmd/trace-output.c > @@ -332,6 +332,74 @@ int tracecmd_ftrace_enable(int set) > return ret; > } > > +__hidden unsigned long long > +out_write_section_header(struct tracecmd_output *handle, unsigned short header_id, > + char *description, enum tracecmd_section_flags flags, bool option) > +{
On Thu, 11 Nov 2021 17:09:00 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > Introduce headers before each file section, in trace file version 7. The > section header has the following format: > <2 bytes>, header ID > <string>, null terminated ASCII string, description of the header We may have discussed this before, but I don't remember. Why did we make the second item a string? I would think we would want all the fixed size portions of the header to be first, and the variable size portion to be last? <2 bytes> header ID <2 bytes> section flags <4 bytes> size of section. Although, is 4 bytes big enough? Perhaps it should be 8 bytes? What happens if a section is bigger than 4 gigs? And then have the string description at the end. Thoughts? -- Steve > <2 bytes>, section flags: > 1: the section is compressed > <4 bytes>, size of the section > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > include/trace-cmd/trace-cmd.h | 5 ++ > .../include/private/trace-cmd-private.h | 1 + > lib/trace-cmd/include/trace-cmd-local.h | 5 ++ > lib/trace-cmd/trace-output.c | 68 +++++++++++++++++++ > 4 files changed, 79 insertions(+) >
On Sat, Dec 4, 2021 at 3:35 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 11 Nov 2021 17:09:00 +0200 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > Introduce headers before each file section, in trace file version 7. The > > section header has the following format: > > <2 bytes>, header ID > > <string>, null terminated ASCII string, description of the header > > We may have discussed this before, but I don't remember. Why did we make > the second item a string? I would think we would want all the fixed size > portions of the header to be first, and the variable size portion to be > last? > The only reason to have a string is for "trace-cmd dump" - to be able to dump some description for unknown sections, we discussed that in the past. But now when I'm looking at that again - there is no sense to have it only for that reason, the ID is enough. I can remove the string from the header. > <2 bytes> header ID > <2 bytes> section flags > <4 bytes> size of section. > > Although, is 4 bytes big enough? Perhaps it should be 8 bytes? What happens > if a section is bigger than 4 gigs? agree, I will make it 8 bytes in the next version. > > And then have the string description at the end. Thoughts? > > -- Steve > > > > <2 bytes>, section flags: > > 1: the section is compressed > > <4 bytes>, size of the section > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- > > include/trace-cmd/trace-cmd.h | 5 ++ > > .../include/private/trace-cmd-private.h | 1 + > > lib/trace-cmd/include/trace-cmd-local.h | 5 ++ > > lib/trace-cmd/trace-output.c | 68 +++++++++++++++++++ > > 4 files changed, 79 insertions(+) > >
On Mon, 6 Dec 2021 05:25:56 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > The only reason to have a string is for "trace-cmd dump" - to be able > to dump some description for unknown sections, we discussed that in > the past. But now when I'm looking at that again - there is no sense > to have it only for that reason, the ID is enough. I can remove the > string from the header. I think we also discussed it for when a new section was added that wasn't known to the current trace-cmd, that it could give a more useful message. i.e. "This version of trace-cmd does not handle 'New feature here', please upgrade your trace-cmd". That way, the user would at least know why that new feature is not being processed. -- Steve
On Mon, Dec 6, 2021 at 4:55 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 6 Dec 2021 05:25:56 +0200 > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > > > The only reason to have a string is for "trace-cmd dump" - to be able > > to dump some description for unknown sections, we discussed that in > > the past. But now when I'm looking at that again - there is no sense > > to have it only for that reason, the ID is enough. I can remove the > > string from the header. > > I think we also discussed it for when a new section was added that wasn't > known to the current trace-cmd, that it could give a more useful message. > i.e. "This version of trace-cmd does not handle 'New feature here', please > upgrade your trace-cmd". > > That way, the user would at least know why that new feature is not being > processed. > As we discussed, there are two possible approaches for these descriptions: 1. To add a new section with these descriptions, that way there will be no strings in the section headers. I was thinking more about that approach, it will make the parsing much more complicated. As all sections are dynamically located in the file, there is no strict order - that "description" section can be anywhere, even at the end. That way during the parsing these strings will not be available for the sections before, and the description cannot be displayed. To solve that, parsing should be done in two steps - first to find that "description" section to get all the strings, and second to parse the other sections. The description should be part of the section being parsed, that's why I decided to go with the second approach. 2. Have a fixed size description string in the section header, I think 32 bytes should be OK. > -- Steve
On Mon, 6 Dec 2021 17:52:54 +0200 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > As we discussed, there are two possible approaches for these descriptions: > 1. To add a new section with these descriptions, that way there will > be no strings in the section headers. I was thinking more about that > approach, it will make the parsing much more complicated. As all > sections are dynamically located in the file, there is no strict order > - that "description" section can be anywhere, even at the end. That > way during the parsing these strings will not be available for the > sections before, and the description cannot be displayed. To solve > that, parsing should be done in two steps - first to find that > "description" section to get all the strings, and second to parse the > other sections. The description should be part of the section being > parsed, that's why I decided to go with the second approach. Again, I would look at the ELF file format for answers ;-) The "string table" is a special section that is described in the main header. That way, the first thing it does, is to to look for this table. It scans all the section headers to find the one that points to the strings, and then loads that one first. For string "pointers", the pointer is not an offset into the file, but an offset into the section (uncompressed). So for reading the file, yes, we would need to first find the string section, and load all the strings into memory. Then go back and read the normal sections. Then we have access for all the strings. I would also say that the string section is optional, and a file could be created with no strings, as they are for description purposes only, and in that case, we need to just say "no strings found" for anything that wants to know the string. -- Steve > 2. Have a fixed size description string in the section header, I think > 32 bytes should be OK.
diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h index 7fea4e01..5d71e8ba 100644 --- a/include/trace-cmd/trace-cmd.h +++ b/include/trace-cmd/trace-cmd.h @@ -15,6 +15,11 @@ enum tracecmd_open_flags { TRACECMD_FL_LOAD_NO_PLUGINS = 1 << 0, /* Do not load plugins */ TRACECMD_FL_LOAD_NO_SYSTEM_PLUGINS = 1 << 1, /* Do not load system plugins */ }; + +enum tracecmd_section_flags { + TRACECMD_SEC_FL_COMPRESS = 1 << 0, /* the section is compressed */ +}; + struct tracecmd_input *tracecmd_open_head(const char *file, int flags); struct tracecmd_input *tracecmd_open(const char *file, int flags); struct tracecmd_input *tracecmd_open_fd(int fd, int flags); diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h index eaa902a3..5b253511 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_MAX, }; enum { diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h index d65ff599..d3760483 100644 --- a/lib/trace-cmd/include/trace-cmd-local.h +++ b/lib/trace-cmd/include/trace-cmd-local.h @@ -37,6 +37,11 @@ struct data_file_write { bool check_file_state(unsigned long file_version, int current_state, int new_state); bool check_out_state(struct tracecmd_output *handle, int new_state); +unsigned long long +out_write_section_header(struct tracecmd_output *handle, unsigned short header_id, + char *description, enum tracecmd_section_flags flags, bool option); +int out_update_section_header(struct tracecmd_output *handle, unsigned long long offset); + struct cpu_data_source { int fd; int size; diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c index e8c788ad..a8c1305d 100644 --- a/lib/trace-cmd/trace-output.c +++ b/lib/trace-cmd/trace-output.c @@ -332,6 +332,74 @@ int tracecmd_ftrace_enable(int set) return ret; } +__hidden unsigned long long +out_write_section_header(struct tracecmd_output *handle, unsigned short header_id, + char *description, enum tracecmd_section_flags flags, bool option) +{ + tsize_t endian8; + tsize_t offset; + short endian2; + int size; + + if (header_id >= TRACECMD_OPTION_MAX) + return -1; + if (!HAS_SECTIONS(handle)) + return 0; + offset = do_lseek(handle, 0, SEEK_CUR); + if (option) { + endian8 = convert_endian_8(handle, offset); + if (!tracecmd_add_option(handle, header_id, 8, &endian8)) + return -1; + } + /* Section ID */ + endian2 = convert_endian_2(handle, header_id); + if (do_write_check(handle, &endian2, 2)) + return (off_t)-1; + + /* Section description */ + if (do_write_check(handle, description, strlen(description) + 1)) + return (off_t)-1; + /* Section flags */ + endian2 = convert_endian_2(handle, flags); + if (do_write_check(handle, &endian2, 2)) + return (off_t)-1; + + offset = do_lseek(handle, 0, SEEK_CUR); + size = 0; + /* Reserve for section size */ + if (do_write_check(handle, &size, 4)) + return (off_t)-1; + return offset; +} + +__hidden int out_update_section_header(struct tracecmd_output *handle, unsigned long long offset) +{ + unsigned long long current; + unsigned int endian4; + int size; + + if (!HAS_SECTIONS(handle) || offset == 0) + return 0; + + current = do_lseek(handle, 0, SEEK_CUR); + /* The real size is the difference between the saved offset and + * the current offset - 4 bytes, the reserved space for the section size. + */ + size = current - offset; + if (size < 4) + return -1; + size -= 4; + if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1) + return -1; + + endian4 = convert_endian_4(handle, size); + if (do_write_check(handle, &endian4, 4)) + return -1; + if (do_lseek(handle, current, SEEK_SET) == (off_t)-1) + return -1; + return 0; +} + static int read_header_files(struct tracecmd_output *handle) { tsize_t size, check_size, endian8;
Introduce headers before each file section, in trace file version 7. The section header has the following format: <2 bytes>, header ID <string>, null terminated ASCII string, description of the header <2 bytes>, section flags: 1: the section is compressed <4 bytes>, size of the section Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- include/trace-cmd/trace-cmd.h | 5 ++ .../include/private/trace-cmd-private.h | 1 + lib/trace-cmd/include/trace-cmd-local.h | 5 ++ lib/trace-cmd/trace-output.c | 68 +++++++++++++++++++ 4 files changed, 79 insertions(+)