| Message ID | 20190617115218.6279-2-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Save tracee memory map into trace.dat file | expand |
On Mon, 17 Jun 2019 14:52:17 +0300 tz.stoyanov@gmail.com wrote: > From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> > > From: Tzvetomir Stoyanov <tstoyanov@vmware.com> > > This patch implements a new tracecmd API, tracecmd_add_option_v() > It adds new option in trace.dat, similar to tracecmd_add_option(), > but the option's data is passed as list of buffers. The standard > struct iovec is used as input parameter, containing the option's > data buffers. > > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> > --- > include/trace-cmd/trace-cmd.h | 5 ++ > include/traceevent/event-parse.h | 1 + > tracecmd/trace-output.c | 117 ++++++++++++++++++++++++++----- > 3 files changed, 106 insertions(+), 17 deletions(-) > > diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h > index ceb03f4..3919673 100644 > --- a/include/trace-cmd/trace-cmd.h > +++ b/include/trace-cmd/trace-cmd.h > @@ -245,11 +245,16 @@ struct tracecmd_output *tracecmd_create_init_file_override(const char *output_fi > struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle, > unsigned short id, int size, > const void *data); > +struct tracecmd_option * > +tracecmd_add_option_v(struct tracecmd_output *handle, > + unsigned short id, const struct iovec *vector, int count); > + > struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle, > const char *name, int cpus); > > int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus); > int tracecmd_write_options(struct tracecmd_output *handle); > +int tracecmd_append_options(struct tracecmd_output *handle); The change log doesn't mention anything about tracecmd_append_options()? > int tracecmd_update_option(struct tracecmd_output *handle, > struct tracecmd_option *option, int size, > const void *data); > diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h > index 5e0fd19..62057b3 100644 > --- a/include/traceevent/event-parse.h > +++ b/include/traceevent/event-parse.h > @@ -11,6 +11,7 @@ > #include <stdio.h> > #include <regex.h> > #include <string.h> > +#include <sys/uio.h> > > #include "trace-seq.h" > > diff --git a/tracecmd/trace-output.c b/tracecmd/trace-output.c > index 33d6ce3..f7a2791 100644 > --- a/tracecmd/trace-output.c > +++ b/tracecmd/trace-output.c > @@ -883,21 +883,23 @@ static struct tracecmd_output *create_file(const char *output_file, > } > > /** > - * tracecmd_add_option - add options to the file > + * tracecmd_add_option_v - add options to the file > * @handle: the output file handle name > * @id: the id of the option > - * @size: the size of the option data > - * @data: the data to write to the file. > + * @vector: array of vectors, pointing to the data to write in the file > + * @count: number of items in the vector array > * > * Returns handle to update option if needed. > * Just the content can be updated, with smaller or equal to > * content than the specified size. > */ > struct tracecmd_option * > -tracecmd_add_option(struct tracecmd_output *handle, > - unsigned short id, int size, const void *data) > +tracecmd_add_option_v(struct tracecmd_output *handle, > + unsigned short id, const struct iovec *vector, int count) Hmm, I think this is a bit overkill. I don't really see anything using more than one or two data vectors. All I see would be at most a "count" followed by a list of data, which is what I think you are using this for. I rather wait to implement something like this when there's more of a need for it. I don't believe this change really requires it. -- Steve > { > struct tracecmd_option *option; > + char *data = NULL; > + int i, size = 0; > > /* > * We can only add options before they were written. > @@ -906,32 +908,63 @@ tracecmd_add_option(struct tracecmd_output *handle, > if (handle->options_written) > return NULL; > > - handle->nr_options++; > + for (i = 0; i < count; i++) > + size += vector[i].iov_len; > + > + /* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass vector with 0 / NULL data */ > + if (size) { > + data = malloc(size); > + if (!data) { > + warning("Insufficient memory"); > + return NULL; > + } > + } > > option = malloc(sizeof(*option)); > if (!option) { > warning("Could not allocate space for option"); > + free(data); > return NULL; > } > > - option->id = id; > - option->size = size; > - option->data = malloc(size); > - if (!option->data) { > - warning("Insufficient memory"); > - free(option); > - return NULL; > + handle->nr_options++; > + option->data = data; > + for (i = 0; i < count; i++) { > + if (vector[i].iov_base && vector[i].iov_len) { > + memcpy(data, vector[i].iov_base, vector[i].iov_len); > + data += vector[i].iov_len; > + } > } > - > - /* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass 0 / NULL data */ > - if (size) > - memcpy(option->data, data, size); > + option->size = size; > + option->id = id; > > list_add_tail(&option->list, &handle->options); > > return option; > } > > +/** > + * tracecmd_add_option - add options to the file > + * @handle: the output file handle name > + * @id: the id of the option > + * @size: the size of the option data > + * @data: the data to write to the file. > + * > + * Returns handle to update option if needed. > + * Just the content can be updated, with smaller or equal to > + * content than the specified size. > + */ > +struct tracecmd_option * > +tracecmd_add_option(struct tracecmd_output *handle, > + unsigned short id, int size, const void *data) > +{ > + struct iovec vect; > + > + vect.iov_base = (void *) data; > + vect.iov_len = size; > + return tracecmd_add_option_v(handle, id, &vect, 1); > +} > + > int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus) > { > cpus = convert_endian_4(handle, cpus); > @@ -979,6 +1012,56 @@ int tracecmd_write_options(struct tracecmd_output *handle) > return 0; > } > > +int tracecmd_append_options(struct tracecmd_output *handle) > +{ > + struct tracecmd_option *options; > + unsigned short option; > + unsigned short endian2; > + unsigned int endian4; > + off_t offset; > + int r; > + > + /* If already written, ignore */ > + if (handle->options_written) > + return 0; > + > + if (lseek64(handle->fd, 0, SEEK_END) == (off_t)-1) > + return -1; > + offset = lseek64(handle->fd, -2, SEEK_CUR); > + if (offset == (off_t)-1) > + return -1; > + > + r = pread(handle->fd, &option, 2, offset); > + if (r != 2 || option != TRACECMD_OPTION_DONE) > + return -1; > + > + list_for_each_entry(options, &handle->options, list) { > + 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 in case it needs to be updated */ > + options->offset = lseek64(handle->fd, 0, SEEK_CUR); > + > + if (do_write_check(handle, options->data, > + options->size)) > + return -1; > + } > + > + option = TRACECMD_OPTION_DONE; > + > + if (do_write_check(handle, &option, 2)) > + return -1; > + > + handle->options_written = 1; > + > + return 0; > +} > + > int tracecmd_update_option(struct tracecmd_output *handle, > struct tracecmd_option *option, int size, > const void *data)
Hi Steven On Mon, Jun 17, 2019 at 4:06 PM Steven Rostedt <rostedt@goodmis.org> wrote: > ... > > Hmm, I think this is a bit overkill. I don't really see anything using > more than one or two data vectors. All I see would be at most a "count" > followed by a list of data, which is what I think you are using this > for. > > I rather wait to implement something like this when there's more of a > need for it. I don't believe this change really requires it. > > -- Steve > Actually, this patch is from the patch set (N 7): "trace-cmd: Timetamps sync between host and guest machines, relying on vsock events. " and I took it as-is. In timesync changes tracecmd_add_option_v() is used in similar way. That explains the tracecmd_append_options() API, which is used there.
On Mon, 17 Jun 2019 16:18:38 +0300 Ceco <tz.stoyanov@gmail.com> wrote: > Hi Steven > > On Mon, Jun 17, 2019 at 4:06 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > ... > > > > Hmm, I think this is a bit overkill. I don't really see anything using > > more than one or two data vectors. All I see would be at most a "count" > > followed by a list of data, which is what I think you are using this > > for. > > > > I rather wait to implement something like this when there's more of a > > need for it. I don't believe this change really requires it. > > > > -- Steve > > > > Actually, this patch is from the patch set (N 7): > "trace-cmd: Timetamps sync between host and guest machines, relying > on vsock events. " > and I took it as-is. In timesync changes tracecmd_add_option_v() is > used in similar way. > That explains the tracecmd_append_options() API, which is used there. > > Ah, that needed to be stated. Yeah, when taking a patch from another series, make sure that the change log is updated too. Never expect that the reviewer will know about any other series ;-) -- Steve
diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h index ceb03f4..3919673 100644 --- a/include/trace-cmd/trace-cmd.h +++ b/include/trace-cmd/trace-cmd.h @@ -245,11 +245,16 @@ struct tracecmd_output *tracecmd_create_init_file_override(const char *output_fi struct tracecmd_option *tracecmd_add_option(struct tracecmd_output *handle, unsigned short id, int size, const void *data); +struct tracecmd_option * +tracecmd_add_option_v(struct tracecmd_output *handle, + unsigned short id, const struct iovec *vector, int count); + struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus); int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus); int tracecmd_write_options(struct tracecmd_output *handle); +int tracecmd_append_options(struct tracecmd_output *handle); int tracecmd_update_option(struct tracecmd_output *handle, struct tracecmd_option *option, int size, const void *data); diff --git a/include/traceevent/event-parse.h b/include/traceevent/event-parse.h index 5e0fd19..62057b3 100644 --- a/include/traceevent/event-parse.h +++ b/include/traceevent/event-parse.h @@ -11,6 +11,7 @@ #include <stdio.h> #include <regex.h> #include <string.h> +#include <sys/uio.h> #include "trace-seq.h" diff --git a/tracecmd/trace-output.c b/tracecmd/trace-output.c index 33d6ce3..f7a2791 100644 --- a/tracecmd/trace-output.c +++ b/tracecmd/trace-output.c @@ -883,21 +883,23 @@ static struct tracecmd_output *create_file(const char *output_file, } /** - * tracecmd_add_option - add options to the file + * tracecmd_add_option_v - add options to the file * @handle: the output file handle name * @id: the id of the option - * @size: the size of the option data - * @data: the data to write to the file. + * @vector: array of vectors, pointing to the data to write in the file + * @count: number of items in the vector array * * Returns handle to update option if needed. * Just the content can be updated, with smaller or equal to * content than the specified size. */ struct tracecmd_option * -tracecmd_add_option(struct tracecmd_output *handle, - unsigned short id, int size, const void *data) +tracecmd_add_option_v(struct tracecmd_output *handle, + unsigned short id, const struct iovec *vector, int count) { struct tracecmd_option *option; + char *data = NULL; + int i, size = 0; /* * We can only add options before they were written. @@ -906,32 +908,63 @@ tracecmd_add_option(struct tracecmd_output *handle, if (handle->options_written) return NULL; - handle->nr_options++; + for (i = 0; i < count; i++) + size += vector[i].iov_len; + + /* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass vector with 0 / NULL data */ + if (size) { + data = malloc(size); + if (!data) { + warning("Insufficient memory"); + return NULL; + } + } option = malloc(sizeof(*option)); if (!option) { warning("Could not allocate space for option"); + free(data); return NULL; } - option->id = id; - option->size = size; - option->data = malloc(size); - if (!option->data) { - warning("Insufficient memory"); - free(option); - return NULL; + handle->nr_options++; + option->data = data; + for (i = 0; i < count; i++) { + if (vector[i].iov_base && vector[i].iov_len) { + memcpy(data, vector[i].iov_base, vector[i].iov_len); + data += vector[i].iov_len; + } } - - /* Some IDs (like TRACECMD_OPTION_TRACECLOCK) pass 0 / NULL data */ - if (size) - memcpy(option->data, data, size); + option->size = size; + option->id = id; list_add_tail(&option->list, &handle->options); return option; } +/** + * tracecmd_add_option - add options to the file + * @handle: the output file handle name + * @id: the id of the option + * @size: the size of the option data + * @data: the data to write to the file. + * + * Returns handle to update option if needed. + * Just the content can be updated, with smaller or equal to + * content than the specified size. + */ +struct tracecmd_option * +tracecmd_add_option(struct tracecmd_output *handle, + unsigned short id, int size, const void *data) +{ + struct iovec vect; + + vect.iov_base = (void *) data; + vect.iov_len = size; + return tracecmd_add_option_v(handle, id, &vect, 1); +} + int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus) { cpus = convert_endian_4(handle, cpus); @@ -979,6 +1012,56 @@ int tracecmd_write_options(struct tracecmd_output *handle) return 0; } +int tracecmd_append_options(struct tracecmd_output *handle) +{ + struct tracecmd_option *options; + unsigned short option; + unsigned short endian2; + unsigned int endian4; + off_t offset; + int r; + + /* If already written, ignore */ + if (handle->options_written) + return 0; + + if (lseek64(handle->fd, 0, SEEK_END) == (off_t)-1) + return -1; + offset = lseek64(handle->fd, -2, SEEK_CUR); + if (offset == (off_t)-1) + return -1; + + r = pread(handle->fd, &option, 2, offset); + if (r != 2 || option != TRACECMD_OPTION_DONE) + return -1; + + list_for_each_entry(options, &handle->options, list) { + 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 in case it needs to be updated */ + options->offset = lseek64(handle->fd, 0, SEEK_CUR); + + if (do_write_check(handle, options->data, + options->size)) + return -1; + } + + option = TRACECMD_OPTION_DONE; + + if (do_write_check(handle, &option, 2)) + return -1; + + handle->options_written = 1; + + return 0; +} + int tracecmd_update_option(struct tracecmd_output *handle, struct tracecmd_option *option, int size, const void *data)