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

[Steven Rostedt <rostedt@goodmis.org>]

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.

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

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

> +};
> +
>  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