Re: [PATCH v2 1/2] trace-cmd: Add validation for reading and writing trace.dat files

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

Hi Steven


On Wed, Feb 24, 2021 at 12:18 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 19 Feb 2021 07:31:55 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > trace.dat files have multiple sections, which must be in strict order. A
> > new logic is implemented, which checks the order of all mandatory
> > sections when reading and writing trace files. This validation is
> > useful when the file is constructed in different machines -
> > host / guest or listener tracing. In those use cases, part of the file
> > is generated in the client machine and is transferred to the server as
> > a sequence of bytes. The server should validate the format of the received
> > portion of the file and the order of the sections in it.
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
>
> >  /* --- Opening and Reading the trace.dat file --- */
> >
> > +enum  {
> > +     TRACECMD_FILE_INIT,
> > +     TRACECMD_FILE_HEADERS,
> > +     TRACECMD_FILE_FTRACE_EVENTS,
> > +     TRACECMD_FILE_ALL_EVENTS,
> > +     TRACECMD_FILE_KALLSYMS,
> > +     TRACECMD_FILE_PRINTK,
> > +     TRACECMD_FILE_CMD_LINES,
> > +     TRACECMD_FILE_CPU_COUNT,
> > +     TRACECMD_FILE_OPTIONS,
> > +     TRACECMD_FILE_CPU_LATENCY,
> > +     TRACECMD_FILE_CPU_FLYRECORD,
>
> I still really don't think we want to make LATENCY and FLYRECORD states.
> Because they are not a state of the trace.dat file, but a type.

I considered these as states, as any of the previous states, that indicate
what kind of data is currently in the file. We can replace both with a single
TRACECMD_FILE_CPU_DATA state, and use the old TRACECMD_FL_
flags to find what kind of tracing data is in the file.

>
> Unless we document here that they are the last states of the file, and once
> reached, the state can not change.

This is true for the current tarce.dat file format - they are the last states
and as for the all other states - once reached, previously written data
cannot be changed. May be I cannot understand your point here, may
be you mean that once these final states are reached, the tracing data
is still not written in the file (read from the file) ? In that case may be it
is more logical to add additional state, to indicate that all trace data is
in the file, regardless of its type (cpu / latency) ?

>
> But is that the case? We may want states about reading
>

I use the same logic for both reading and writing the file. When reading
a file and if one of the TRACECMD_FILE_CPU_ states is reached, the
tracing data is ready to be read.

> > +};
> > +
> >  enum {
> >       TRACECMD_OPTION_DONE,
> >       TRACECMD_OPTION_DATE,
> > @@ -115,9 +129,7 @@ enum {
> >  enum {
> >       TRACECMD_FL_IGNORE_DATE         = (1 << 0),
> >       TRACECMD_FL_BUFFER_INSTANCE     = (1 << 1),
> > -     TRACECMD_FL_LATENCY             = (1 << 2),
> > -     TRACECMD_FL_IN_USECS            = (1 << 3),
> > -     TRACECMD_FL_FLYRECORD           = (1 << 4),
> > +     TRACECMD_FL_IN_USECS            = (1 << 2),
> >  };
> >
>
> > @@ -2665,9 +2678,9 @@ static int read_options_type(struct tracecmd_input *handle)
> >        * Check if this is a latency report or flyrecord.
> >        */
> >       if (strncmp(buf, "latency", 7) == 0)
> > -             handle->flags |= TRACECMD_FL_LATENCY;
> > +             handle->file_state = TRACECMD_FILE_CPU_LATENCY;
> >       else if (strncmp(buf, "flyrecord", 9) == 0)
> > -             handle->flags |= TRACECMD_FL_FLYRECORD;
> > +             handle->file_state = TRACECMD_FILE_CPU_FLYRECORD;
>
> What happens when we change states after this? Or is this going to always
> be the last state of the file?
>
> What if we want to change the state after we read the CPUs, or for the
> latency, we may want to change the state after reading the trace file.
>
> The more I think about this, the more having them be states does not make
> sense. They are the type of file, and should stay as flags.
>
> What benefit do you see for keeping them a state?
>
> -- Steve



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