| Message ID | 20211210105448.97850-8-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:30 +0200 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: Nit, paragraphs should really be separated by spaces ;-) > Introduced chain of options sections in trace file version 7. Extended > the "DONE" option to hold the offset into the file to the next options > section. > Format of trace file version 7 is extended with a new mandatory field > after the compression algorithm header: > <8 bytes>, unsigned long long integer - offset into the trace file > where the first options section is located. This allows to place this > section anywhere in the file. As all other sections have corresponding > options, describing their offsets into the trace file, this change makes > the structure of trace file version 7 flexible. Makes it easier to read. Especially when you get older and your eyes don't work as well. > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > --- > lib/trace-cmd/trace-output.c | 103 ++++++++++++++++++++++++++++++++--- > 1 file changed, 96 insertions(+), 7 deletions(-) > > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c > index aa55aad7..6e07fece 100644 > --- a/lib/trace-cmd/trace-output.c > +++ b/lib/trace-cmd/trace-output.c > @@ -64,7 +64,7 @@ struct tracecmd_output { > unsigned long file_version; > unsigned long strings_p; > unsigned long strings_offs; > - size_t options_start; > + tsize_t options_start; I have to ask. Why use tsize_t? (I see it used elsewhere as well, but just really noticed it now) The only reference to tsize_t I see in the wild is for libtiff. > bool big_endian; > > struct list_head options; > @@ -89,6 +89,7 @@ struct list_event_system { > > #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) > > +static int write_options_v7(struct tracecmd_output *handle); I'm thinking that since v7 will be the main method, we could just remove all the references to "v7", as we don't know if v8 will use this as well. That is, the old versions should be labeled as such ("v6"), but the new versions shouldn't have to be. > static int save_string_section(struct tracecmd_output *handle); > > static stsize_t > @@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle) > return; > > if (handle->file_version >= FILE_VERSION_SECTIONS) { > + /* write any unsaved options at the end of trace files with sections */ > + write_options_v7(handle); > + > /* write strings section */ > save_string_section(handle); > } > @@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version > */ > static int output_write_init(struct tracecmd_output *handle) > { > + unsigned long long offset; > char buf[BUFSIZ]; > int endian4; > > @@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle) > endian4 = convert_endian_4(handle, handle->page_size); > if (do_write_check(handle, &endian4, 4)) > return -1; > + if (HAS_SECTIONS(handle)) { > + /* Write 0 as options offset and save its location */ > + offset = 0; > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > + if (do_write_check(handle, &offset, 8)) > + return -1; > + } > + > handle->file_state = TRACECMD_FILE_INIT; > return 0; > } > @@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, > * We can only add options before tracing data were written. > * This may change in the future. > */ > - if (handle->file_state > TRACECMD_FILE_OPTIONS) > + if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS) > return NULL; > > for (i = 0; i < count; i++) > @@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, > return NULL; > } > } > - > - option = malloc(sizeof(*option)); > + option = calloc(1, sizeof(*option)); > if (!option) { > tracecmd_warning("Could not allocate space for option"); > free(data); > @@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus) > return 0; > } > > -int tracecmd_write_options(struct tracecmd_output *handle) > +static int write_options_v6(struct tracecmd_output *handle) > { > struct tracecmd_option *options; > unsigned short option; > @@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle) > > if (do_write_check(handle, "options ", 10)) > return -1; > - > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > list_for_each_entry(options, &handle->options, list) { > endian2 = convert_endian_2(handle, options->id); > if (do_write_check(handle, &endian2, 2)) > @@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle) > return -1; > > handle->file_state = TRACECMD_FILE_OPTIONS; > + return 0; > +} > + > +static int write_options_v7(struct tracecmd_output *handle) > +{ > + struct tracecmd_option *options; > + unsigned long long endian8; > + unsigned short endian2; > + unsigned int endian4; > + bool new = false; > + tsize_t offset; > + > + /* Check if there are unsaved options */ > + list_for_each_entry(options, &handle->options, list) { > + if (!options->offset) { > + new = true; > + break; > + } > + } > + > + if (!new) > + return 0; > + offset = do_lseek(handle, 0, SEEK_CUR); The spacing is backwards above. You can remove the space before the "if (!new)" and add one after the if condition. The if check is the result of the previous loop, and should be associated with it, just like one would do in any language. Code spacing is important. It should be used to show how lines of code are associated with each other, just like paragraphs in text. > + /* Append to the previous options section, if any */ > + if (handle->options_start) { > + if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1) > + return -1; > + endian8 = convert_endian_8(handle, offset); > + if (do_write_check(handle, &endian8, 8)) > + return -1; > + if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1) > + return -1; > + } space > + offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false); > + if (offset == (off_t)-1) > + return -1; space. > + list_for_each_entry(options, &handle->options, list) { > + /* Option is already saved, skip it */ > + if (options->offset) > + continue; > + endian2 = convert_endian_2(handle, options->id); > + if (do_write_check(handle, &endian2, 2)) > + return -1; > + endian4 = convert_endian_4(handle, options->size); > + if (do_write_check(handle, &endian4, 4)) > + return -1; > + /* Save the data location */ > + options->offset = do_lseek(handle, 0, SEEK_CUR); > + if (do_write_check(handle, options->data, options->size)) > + return -1; > + } > + > + endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE); > + if (do_write_check(handle, &endian2, 2)) > + return -1; > + endian4 = convert_endian_4(handle, 8); > + if (do_write_check(handle, &endian4, 4)) > + return -1; > + endian8 = 0; > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > + if (do_write_check(handle, &endian8, 8)) > + return -1; > + if (out_update_section_header(handle, offset)) > + return -1; > > return 0; > } -- Steve
On Sat, Jan 15, 2022 at 4:57 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 10 Dec 2021 12:54:30 +0200 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > Nit, paragraphs should really be separated by spaces ;-) > > > Introduced chain of options sections in trace file version 7. Extended > > the "DONE" option to hold the offset into the file to the next options > > section. > > > Format of trace file version 7 is extended with a new mandatory field > > after the compression algorithm header: > > > <8 bytes>, unsigned long long integer - offset into the trace file > > where the first options section is located. This allows to place this > > section anywhere in the file. As all other sections have corresponding > > options, describing their offsets into the trace file, this change makes > > the structure of trace file version 7 flexible. > > Makes it easier to read. Especially when you get older and your eyes > don't work as well. > > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- > > lib/trace-cmd/trace-output.c | 103 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 96 insertions(+), 7 deletions(-) > > > > diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c > > index aa55aad7..6e07fece 100644 > > --- a/lib/trace-cmd/trace-output.c > > +++ b/lib/trace-cmd/trace-output.c > > @@ -64,7 +64,7 @@ struct tracecmd_output { > > unsigned long file_version; > > unsigned long strings_p; > > unsigned long strings_offs; > > - size_t options_start; > > + tsize_t options_start; > > I have to ask. Why use tsize_t? (I see it used elsewhere as well, but > just really noticed it now) > > The only reference to tsize_t I see in the wild is for libtiff. That is the comment where tsize_t is defined, at the top of this file: /* We can't depend on the host size for size_t, all must be 64 bit */ As I understand, on 32 bit arch, size_t is 32 bit. "options_start" is offset in the file, it should be always 64 bit. I can just use "unsigned long long" instead. > > > bool big_endian; > > > > struct list_head options; > > @@ -89,6 +89,7 @@ struct list_event_system { > > > > #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) > > > > +static int write_options_v7(struct tracecmd_output *handle); > > I'm thinking that since v7 will be the main method, we could just > remove all the references to "v7", as we don't know if v8 will use this > as well. That is, the old versions should be labeled as such ("v6"), > but the new versions shouldn't have to be. > > > static int save_string_section(struct tracecmd_output *handle); > > > > static stsize_t > > @@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle) > > return; > > > > if (handle->file_version >= FILE_VERSION_SECTIONS) { > > + /* write any unsaved options at the end of trace files with sections */ > > + write_options_v7(handle); > > + > > /* write strings section */ > > save_string_section(handle); > > } > > @@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version > > */ > > static int output_write_init(struct tracecmd_output *handle) > > { > > + unsigned long long offset; > > char buf[BUFSIZ]; > > int endian4; > > > > @@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle) > > endian4 = convert_endian_4(handle, handle->page_size); > > if (do_write_check(handle, &endian4, 4)) > > return -1; > > + if (HAS_SECTIONS(handle)) { > > + /* Write 0 as options offset and save its location */ > > + offset = 0; > > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > > + if (do_write_check(handle, &offset, 8)) > > + return -1; > > + } > > + > > handle->file_state = TRACECMD_FILE_INIT; > > return 0; > > } > > @@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, > > * We can only add options before tracing data were written. > > * This may change in the future. > > */ > > - if (handle->file_state > TRACECMD_FILE_OPTIONS) > > + if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS) > > return NULL; > > > > for (i = 0; i < count; i++) > > @@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, > > return NULL; > > } > > } > > - > > - option = malloc(sizeof(*option)); > > + option = calloc(1, sizeof(*option)); > > if (!option) { > > tracecmd_warning("Could not allocate space for option"); > > free(data); > > @@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus) > > return 0; > > } > > > > -int tracecmd_write_options(struct tracecmd_output *handle) > > +static int write_options_v6(struct tracecmd_output *handle) > > { > > struct tracecmd_option *options; > > unsigned short option; > > @@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle) > > > > if (do_write_check(handle, "options ", 10)) > > return -1; > > - > > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > > list_for_each_entry(options, &handle->options, list) { > > endian2 = convert_endian_2(handle, options->id); > > if (do_write_check(handle, &endian2, 2)) > > @@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle) > > return -1; > > > > handle->file_state = TRACECMD_FILE_OPTIONS; > > + return 0; > > +} > > + > > +static int write_options_v7(struct tracecmd_output *handle) > > +{ > > + struct tracecmd_option *options; > > + unsigned long long endian8; > > + unsigned short endian2; > > + unsigned int endian4; > > + bool new = false; > > + tsize_t offset; > > + > > + /* Check if there are unsaved options */ > > + list_for_each_entry(options, &handle->options, list) { > > + if (!options->offset) { > > + new = true; > > + break; > > + } > > + } > > + > > + if (!new) > > + return 0; > > + offset = do_lseek(handle, 0, SEEK_CUR); > > The spacing is backwards above. You can remove the space before the > "if (!new)" and add one after the if condition. > > The if check is the result of the previous loop, and should be > associated with it, just like one would do in any language. > > Code spacing is important. It should be used to show how lines of code > are associated with each other, just like paragraphs in text. > > > + /* Append to the previous options section, if any */ > > + if (handle->options_start) { > > + if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1) > > + return -1; > > + endian8 = convert_endian_8(handle, offset); > > + if (do_write_check(handle, &endian8, 8)) > > + return -1; > > + if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1) > > + return -1; > > + } > > space > > > + offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false); > > + if (offset == (off_t)-1) > > + return -1; > > space. > > > + list_for_each_entry(options, &handle->options, list) { > > + /* Option is already saved, skip it */ > > + if (options->offset) > > + continue; > > + endian2 = convert_endian_2(handle, options->id); > > + if (do_write_check(handle, &endian2, 2)) > > + return -1; > > + endian4 = convert_endian_4(handle, options->size); > > + if (do_write_check(handle, &endian4, 4)) > > + return -1; > > + /* Save the data location */ > > + options->offset = do_lseek(handle, 0, SEEK_CUR); > > + if (do_write_check(handle, options->data, options->size)) > > + return -1; > > + } > > + > > + endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE); > > + if (do_write_check(handle, &endian2, 2)) > > + return -1; > > + endian4 = convert_endian_4(handle, 8); > > + if (do_write_check(handle, &endian4, 4)) > > + return -1; > > + endian8 = 0; > > + handle->options_start = do_lseek(handle, 0, SEEK_CUR); > > + if (do_write_check(handle, &endian8, 8)) > > + return -1; > > + if (out_update_section_header(handle, offset)) > > + return -1; > > > > return 0; > > } > > -- Steve
diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c index aa55aad7..6e07fece 100644 --- a/lib/trace-cmd/trace-output.c +++ b/lib/trace-cmd/trace-output.c @@ -64,7 +64,7 @@ struct tracecmd_output { unsigned long file_version; unsigned long strings_p; unsigned long strings_offs; - size_t options_start; + tsize_t options_start; bool big_endian; struct list_head options; @@ -89,6 +89,7 @@ struct list_event_system { #define HAS_SECTIONS(H) ((H)->file_version >= FILE_VERSION_SECTIONS) +static int write_options_v7(struct tracecmd_output *handle); static int save_string_section(struct tracecmd_output *handle); static stsize_t @@ -218,6 +219,9 @@ void tracecmd_output_close(struct tracecmd_output *handle) return; if (handle->file_version >= FILE_VERSION_SECTIONS) { + /* write any unsaved options at the end of trace files with sections */ + write_options_v7(handle); + /* write strings section */ save_string_section(handle); } @@ -1295,6 +1299,7 @@ int tracecmd_output_set_version(struct tracecmd_output *handle, int file_version */ static int output_write_init(struct tracecmd_output *handle) { + unsigned long long offset; char buf[BUFSIZ]; int endian4; @@ -1328,6 +1333,14 @@ static int output_write_init(struct tracecmd_output *handle) endian4 = convert_endian_4(handle, handle->page_size); if (do_write_check(handle, &endian4, 4)) return -1; + if (HAS_SECTIONS(handle)) { + /* Write 0 as options offset and save its location */ + offset = 0; + handle->options_start = do_lseek(handle, 0, SEEK_CUR); + if (do_write_check(handle, &offset, 8)) + return -1; + } + handle->file_state = TRACECMD_FILE_INIT; return 0; } @@ -1396,7 +1409,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, * We can only add options before tracing data were written. * This may change in the future. */ - if (handle->file_state > TRACECMD_FILE_OPTIONS) + if (!HAS_SECTIONS(handle) && handle->file_state > TRACECMD_FILE_OPTIONS) return NULL; for (i = 0; i < count; i++) @@ -1409,8 +1422,7 @@ tracecmd_add_option_v(struct tracecmd_output *handle, return NULL; } } - - option = malloc(sizeof(*option)); + option = calloc(1, sizeof(*option)); if (!option) { tracecmd_warning("Could not allocate space for option"); free(data); @@ -1473,7 +1485,7 @@ int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus) return 0; } -int tracecmd_write_options(struct tracecmd_output *handle) +static int write_options_v6(struct tracecmd_output *handle) { struct tracecmd_option *options; unsigned short option; @@ -1491,7 +1503,7 @@ int tracecmd_write_options(struct tracecmd_output *handle) if (do_write_check(handle, "options ", 10)) return -1; - + handle->options_start = do_lseek(handle, 0, SEEK_CUR); list_for_each_entry(options, &handle->options, list) { endian2 = convert_endian_2(handle, options->id); if (do_write_check(handle, &endian2, 2)) @@ -1515,6 +1527,70 @@ int tracecmd_write_options(struct tracecmd_output *handle) return -1; handle->file_state = TRACECMD_FILE_OPTIONS; + return 0; +} + +static int write_options_v7(struct tracecmd_output *handle) +{ + struct tracecmd_option *options; + unsigned long long endian8; + unsigned short endian2; + unsigned int endian4; + bool new = false; + tsize_t offset; + + /* Check if there are unsaved options */ + list_for_each_entry(options, &handle->options, list) { + if (!options->offset) { + new = true; + break; + } + } + + if (!new) + return 0; + offset = do_lseek(handle, 0, SEEK_CUR); + /* Append to the previous options section, if any */ + if (handle->options_start) { + if (do_lseek(handle, handle->options_start, SEEK_SET) == (off64_t)-1) + return -1; + endian8 = convert_endian_8(handle, offset); + if (do_write_check(handle, &endian8, 8)) + return -1; + if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1) + return -1; + } + offset = out_write_section_header(handle, TRACECMD_OPTION_DONE, "options", 0, false); + if (offset == (off_t)-1) + return -1; + list_for_each_entry(options, &handle->options, list) { + /* Option is already saved, skip it */ + if (options->offset) + continue; + endian2 = convert_endian_2(handle, options->id); + if (do_write_check(handle, &endian2, 2)) + return -1; + endian4 = convert_endian_4(handle, options->size); + if (do_write_check(handle, &endian4, 4)) + return -1; + /* Save the data location */ + options->offset = do_lseek(handle, 0, SEEK_CUR); + if (do_write_check(handle, options->data, options->size)) + return -1; + } + + endian2 = convert_endian_2(handle, TRACECMD_OPTION_DONE); + if (do_write_check(handle, &endian2, 2)) + return -1; + endian4 = convert_endian_4(handle, 8); + if (do_write_check(handle, &endian4, 4)) + return -1; + endian8 = 0; + handle->options_start = do_lseek(handle, 0, SEEK_CUR); + if (do_write_check(handle, &endian8, 8)) + return -1; + if (out_update_section_header(handle, offset)) + return -1; return 0; } @@ -1527,8 +1603,14 @@ int tracecmd_write_meta_strings(struct tracecmd_output *handle) return save_string_section(handle); } +int tracecmd_write_options(struct tracecmd_output *handle) +{ + if (!HAS_SECTIONS(handle)) + return write_options_v6(handle); + return write_options_v7(handle); +} -int tracecmd_append_options(struct tracecmd_output *handle) +static int append_options_v6(struct tracecmd_output *handle) { struct tracecmd_option *options; unsigned short option; @@ -1579,6 +1661,13 @@ int tracecmd_append_options(struct tracecmd_output *handle) return 0; } +int tracecmd_append_options(struct tracecmd_output *handle) +{ + if (!HAS_SECTIONS(handle)) + return append_options_v6(handle); + return write_options_v7(handle); +} + static struct tracecmd_option * add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus) {
Introduced chain of options sections in trace file version 7. Extended the "DONE" option to hold the offset into the file to the next options section. Format of trace file version 7 is extended with a new mandatory field after the compression algorithm header: <8 bytes>, unsigned long long integer - offset into the trace file where the first options section is located. This allows to place this section anywhere in the file. As all other sections have corresponding options, describing their offsets into the trace file, this change makes the structure of trace file version 7 flexible. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- lib/trace-cmd/trace-output.c | 103 ++++++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 7 deletions(-)