Re: [PATCH v7 07/25] trace-cmd library: Add multiple options sections in trace file version 7

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

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



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center