Re: [PATCH v2 20/87] trace-cmd library: Compress part of the trace file

[Steven Rostedt <rostedt@xxxxxxxxxxx>]

On Thu, 29 Jul 2021 08:08:52 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> @@ -384,26 +384,30 @@ static int read_header_files(struct tracecmd_output *handle)
>  	if (!path)
>  		return -1;
>  
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = stat(path, &st);
>  	if (ret < 0) {


> -
> +	if (compress && out_compression_end(handle))
> +		goto out_close;
>  	handle->file_state = TRACECMD_FILE_HEADERS;
>  


> -
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = copy_event_system(handle, systems);
> -
> +	if (compress) {
> +		if (!ret)
> +			ret = out_compression_end(handle);
> +		else
> +			out_compression_reset(handle);
> +	}


> -
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = -1;
>  	endian4 = convert_endian_4(handle, count);
>  	if (do_write_check(handle, &endian4, 4))
> @@ -763,9 +777,19 @@ static int read_event_files(struct tracecmd_output *handle,
>  		}
>  		ret = copy_event_system(handle, slist);
>  	}
> -
> -	handle->file_state = TRACECMD_FILE_ALL_EVENTS;
> +	if (ret)
> +		goto out_free;
> +	if (compress) {
> +		ret = out_compression_end(handle);
> +		if (ret)
> +			goto out_free;
> +	}


> @@ -827,19 +851,21 @@ static int read_proc_kallsyms(struct tracecmd_output *handle)
>  	if (handle->kallsyms)
>  		path = handle->kallsyms;
>  
> +	if (compress)
> +		out_compression_start(handle);


> -
> -	return 0;
> +	if (compress) {
> +		ret = out_compression_end(handle);
> +		if (ret)
> +			goto out;
> +	}

>  
> +	if (compress)
> +		out_compression_start(handle);
>  	ret = stat(path, &st);
>  	if (ret < 0) {
>  		/* not found */
> @@ -894,11 +931,14 @@ static int read_ftrace_printk(struct tracecmd_output *handle)
>  	}
>  
>   out:
> -	handle->file_state = TRACECMD_FILE_PRINTK;
>  	put_tracing_file(path);
> +	if (compress && out_compression_end(handle))
> +		return -1;
> +	handle->file_state = TRACECMD_FILE_PRINTK;

>  	}
> +
> +	if (handle->compress)
> +		out_compression_start(handle);
> +
>  	ret = save_tracing_file_data(handle, "saved_cmdlines");
> -	if (ret < 0)
> +	if (ret < 0) {
> +		out_compression_reset(handle);
>  		return ret;
> +	}
> +
> +	if (handle->compress && out_compression_end(handle))
> +		return -1;
> +
>  	handle->file_state = TRACECMD_FILE_CMD_LINES;
>  	return 0;
>  }


Pretty much all the out_compression_*() functions are enclosed in a:

	if (compress)
		out_compress_*()

condition.

Why not just add that to the parameter, and clean up the code where
this is used, because these branches are rather ugly and break up the
flow.

For example:

__hidden void out_compression_reset(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return;
        tracecmd_compress_reset(handle->compress);
        handle->do_compress = false;
}

__hidden int out_uncompress_block(struct tracecmd_output *handle, bool compress)
{
        int ret = 0;

        if (!compress || !handle->compress)
                return 0;
        ret = tracecmd_uncompress_block(handle->compress);
        if (!ret)
                handle->do_compress = true;
        return ret;
}

__hidden int out_compression_start(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return 0;
        tracecmd_compress_reset(handle->compress);
        handle->do_compress = true;
        return 0;
}

__hidden int out_compression_end(struct tracecmd_output *handle, bool compress)
{
        if (!compress || !handle->compress)
                return 0;
        handle->do_compress = false;
        return tracecmd_compress_block(handle->compress);
}

Then instead of all these:

        if (compress)
                out_compression_start(handle);

You just have:

        out_compression_start(handle, compress);


-- Steve