On Thu, 29 Jul 2021 08:09:06 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> Trace file version 7 is based on sections. Added an internal sections
> database and new helper functions to add, read, open and close file
> sections.
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> lib/trace-cmd/trace-input.c | 71 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 6872f6ef..e8eefb5c 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -114,6 +114,14 @@ struct tsc2nsec {
> unsigned long long offset;
> };
>
> +struct file_section {
> + int id;
> + unsigned long long section_offset;
> + unsigned long long data_offset;
Should we have a size too?
> + enum tracecmd_section_flags flags;
> + struct file_section *next;
> +};
> +
> struct tracecmd_input {
> struct tep_handle *pevent;
> unsigned long file_state;
> @@ -154,6 +162,7 @@ struct tracecmd_input {
> struct hook_list *hooks;
> struct pid_addr_maps *pid_maps;
> /* file information */
> + struct file_section *sections;
> size_t header_files_start;
> size_t ftrace_files_start;
> size_t event_files_start;
> @@ -411,6 +420,61 @@ __hidden int in_uncompress_block(struct tracecmd_input *handle)
> return ret;
> }
>
> +static struct file_section *get_section(struct tracecmd_input *handle, int id)
> +{
> + struct file_section *sec = handle->sections;
> +
> + while (sec) {
> + if (sec->id == id)
> + return sec;
> + sec = sec->next;
> + }
The above could be a simple for loop.
for (sec = handle->sections; sec; sec = sec->next) {
if (sec->id == id);
return sec;
}
> + return NULL;
> +}
> +
> +static struct file_section *open_section(struct tracecmd_input *handle, int id)
I wonder if we should change the names to "section_open()", "section_close()" ...
This way the prefix "section_" is know to be a command for handling sections.
> +{
> + struct file_section *sec = get_section(handle, id);
> +
> + if (!sec)
> + return NULL;
> +
> + if (lseek64(handle->fd, sec->data_offset, SEEK_SET) == (off64_t)-1)
> + return NULL;
> + if ((sec->flags & TRACECMD_SEC_FL_COMPRESS) && in_uncompress_block(handle))
> + return NULL;
> + return sec;
> +}
> +
> +static void close_section(struct tracecmd_input *handle, struct file_section *sec)
> +{
> + if (sec->flags & TRACECMD_SEC_FL_COMPRESS)
> + in_uncompress_reset(handle);
> +}
> +
> +static int add_section(struct tracecmd_input *handle, int id, int flags,
> + unsigned long long soffset, unsigned long long doffset)
I'm still not sure what "soffest' is compared to "doffset".
> +{
> + struct file_section *sec = get_section(handle, id);
> +
> + if (!sec) {
> + sec = calloc(1, sizeof(struct file_section));
> + if (!sec)
> + return -1;
> + sec->next = handle->sections;
> + handle->sections = sec;
> + }
Hmm, we should probably have an "add" and "update" because blindly
changing an existing section may not be what was expected. That is,
going with the "section_" prefix.
section_add(...)
Would error if it already exists.
section_update(..)
Would expect it to already exist.
Could have an:
section_add_or_update(...)
Which would be the above, and lets the caller know that it will ether
add the section, or update it.
-- Steve
> + sec->id = id;
> + if (soffset)
> + sec->section_offset = soffset;
> + if (doffset)
> + sec->data_offset = doffset;
> + if (flags > 0)
> + sec->flags = flags;
> + return 0;
> +}
> +
> +
> static int read_header_files(struct tracecmd_input *handle)
> {
> struct tep_handle *pevent = handle->pevent;
> @@ -3522,6 +3586,7 @@ void tracecmd_ref(struct tracecmd_input *handle)
> */
> void tracecmd_close(struct tracecmd_input *handle)
> {
> + struct file_section *del_sec;
> int i;
>
> if (!handle)
> @@ -3560,6 +3625,11 @@ void tracecmd_close(struct tracecmd_input *handle)
> free(handle->version);
> close(handle->fd);
>
> + while (handle->sections) {
> + del_sec = handle->sections;
> + handle->sections = handle->sections->next;
> + free(del_sec);
> + }
> for (i = 0; i < handle->nr_buffers; i++)
> free(handle->buffers[i].name);
>
> @@ -4006,6 +4076,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx)
> new_handle->nr_buffers = 0;
> new_handle->buffers = NULL;
> new_handle->version = NULL;
> + new_handle->sections = NULL;
> new_handle->guest = NULL;
> new_handle->ref = 1;
> if (handle->trace_clock) {
|