| Message ID | 20211201022431.64763-1-mikesart@fastmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd library: Add API to get information from trace file | expand |
On Tue, 30 Nov 2021 19:24:31 -0700 Michael Sartain <mikesart@fastmail.com> wrote: > New APIs added to get trace cpustats, uname, version strings, and > cpu file sizes and offsets. Hi Michael, Thanks for contributing! I have some comments below. > > const char *tracecmd_get_cpustats(struct tracecmd_input *handle); > const char *tracecmd_get_uname(struct tracecmd_input *handle); > const char *tracecmd_get_version(struct tracecmd_input *handle); > unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); > unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); > > Signed-off-by: Michael Sartain <mikesart@fastmail.com> > --- > .../include/private/trace-cmd-private.h | 6 +++++ > lib/trace-cmd/trace-input.c | 25 +++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h > index c58b09e..cd78cc9 100644 > --- a/lib/trace-cmd/include/private/trace-cmd-private.h > +++ b/lib/trace-cmd/include/private/trace-cmd-private.h > @@ -90,6 +90,12 @@ bool tracecmd_get_quiet(struct tracecmd_output *handle); > void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock); > const char *tracecmd_get_trace_clock(struct tracecmd_input *handle); > > +const char *tracecmd_get_cpustats(struct tracecmd_input *handle); > +const char *tracecmd_get_uname(struct tracecmd_input *handle); > +const char *tracecmd_get_version(struct tracecmd_input *handle); > +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); > +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); I wonder if we should follow normal standards here and return off64_t type instead of unsigned long long? As that is what is returned by lseek64(). > + > static inline int tracecmd_host_bigendian(void) > { > unsigned char str[] = { 0x1, 0x2, 0x3, 0x4 }; > diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c > index ac57bc4..99dbca2 100644 > --- a/lib/trace-cmd/trace-input.c > +++ b/lib/trace-cmd/trace-input.c > @@ -4088,6 +4088,31 @@ const char *tracecmd_get_trace_clock(struct tracecmd_input *handle) > return handle->trace_clock; > } > For all APIs, we require a "kernel doc" style comment. That is: /** * func_name - one line short description * @var1: short description of var1 * @var2: short description of var2 * * Long description of function, where referencing any variables used * still adds the '@', like @var1 is for X and @var2 is for y. Here * is where a longer description of the variables can be done too. * * Returns: Explain what it returns, and if applicable what function needs * to be called to free the return value. */ For example, for the below function, you can have something like: /** * tracecmd_get_cpustats - return cpustats of input descriptor * @handle: Input descriptor to get cpustats from. * * Provides a method to extract the cpustats saved in @handle. * * Returns: The cpustats string saved in the file @handle. The value * returned must not be freed, as it is part of the descriptor, and will * be freed by the destruction of the @handle. */ > +const char *tracecmd_get_cpustats(struct tracecmd_input *handle) > +{ > + return handle->cpustats; > +} > + And the rest of the functions also need a kernel doc. Not to mention, we would need to add a man page to describe these too. See the Documentation/libtracecmd directory for examples. > +const char *tracecmd_get_uname(struct tracecmd_input *handle) > +{ > + return handle->uname; > +} > + > +const char *tracecmd_get_version(struct tracecmd_input *handle) > +{ > + return handle->version; > +} > + > +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu) Should be "unsigned int cpu" > +{ > + return (cpu < handle->cpus) ? handle->cpu_data[cpu].file_offset : 0; Otherwise if we pass in -1 then the above would return handle->cpu_data[-1] > +} > + > +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu) > +{ > + return (cpu < handle->cpus) ? handle->cpu_data[cpu].file_size : 0; Same here. > +} > + > /** > * tracecmd_get_show_data_func - return the show data func > * @handle: input handle for the trace.dat file Anyway, welcome to our community Michael! -- Steve
On Wed, Dec 01, 2021 at 10:33:22AM -0500, Steven Rostedt wrote: > On Tue, 30 Nov 2021 19:24:31 -0700 > Michael Sartain <mikesart@fastmail.com> wrote: > > > New APIs added to get trace cpustats, uname, version strings, and > > cpu file sizes and offsets. ... > > +const char *tracecmd_get_cpustats(struct tracecmd_input *handle); > > +const char *tracecmd_get_uname(struct tracecmd_input *handle); > > +const char *tracecmd_get_version(struct tracecmd_input *handle); > > +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); > > +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); > > I wonder if we should follow normal standards here and return off64_t type > instead of unsigned long long? As that is what is returned by lseek64(). Happy to make this change. Just to be clear, several ts and cursor functions already in trace-cmd-private.h take "long long" parameters for trace_ids, timestamps, offsets, time, etc. Do you have longer term plans of migrating other things to off64_t? > For all APIs, we require a "kernel doc" style comment. That is: > And the rest of the functions also need a kernel doc. Sounds good, I'm on it. > Not to mention, we would need to add a man page to describe these too. See > the Documentation/libtracecmd directory for examples. This code is all in include/private/trace-cmd-private.h and it doesn't look like any of the existing functions (like tracecmd_get_trace_clock) have man pages. Seems like the trace-cmd man pages all describe the trace-cmd executable? For background, we're wanting to add these functions to this private header file since we're pulling trace-input.c, trace-hooks.c, and trace-util.c into our binary (along with libtracevent) to read events from trace.dat files. Right now tracecmd_input struct members (like uname) are local to trace-input.c and only externally visible via tracecmd_print_uname(). Which isn't useful to a gui app, natch. But yeah, please let me know how you'd like to proceed. Happy to start a man page for trace-cmd-private.h but pretty sure I'm not the best person to document *all* the current functions in this file. :) > > +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu) > > Should be "unsigned int cpu" There are a several existing functions that take an "int cpu" parameter. Is this something where this new functions should use an unsigned int and then update the others later, or perhaps add a check in the code for negative cpu values and keep the "int cpu" parameter to stay consistent with the others? > Anyway, welcome to our community Michael! Thanks for the feedback and warm welcome Steven - much appreciated! Regards, -Mike
On Wed, 1 Dec 2021 19:36:55 -0700 Michael Sartain <mikesart@fastmail.com> wrote: > On Wed, Dec 01, 2021 at 10:33:22AM -0500, Steven Rostedt wrote: > > On Tue, 30 Nov 2021 19:24:31 -0700 > > Michael Sartain <mikesart@fastmail.com> wrote: > > > > > New APIs added to get trace cpustats, uname, version strings, and > > > cpu file sizes and offsets. > > ... > > > > +const char *tracecmd_get_cpustats(struct tracecmd_input *handle); > > > +const char *tracecmd_get_uname(struct tracecmd_input *handle); > > > +const char *tracecmd_get_version(struct tracecmd_input *handle); > > > +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); > > > +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); > > > > I wonder if we should follow normal standards here and return off64_t type > > instead of unsigned long long? As that is what is returned by lseek64(). > > Happy to make this change. Just to be clear, several ts and cursor > functions already in trace-cmd-private.h take "long long" parameters for > trace_ids, timestamps, offsets, time, etc. Do you have longer term plans > of migrating other things to off64_t? Not for the trace_ids or timestamps. But perhaps the offsets should change. I noticed that this is for file size and offset, which off64_t is commonly used for. I didn't mean it to be for all unsigned long long conversions. > > > For all APIs, we require a "kernel doc" style comment. That is: > > And the rest of the functions also need a kernel doc. > > Sounds good, I'm on it. Awesome. > > > Not to mention, we would need to add a man page to describe these too. See > > the Documentation/libtracecmd directory for examples. > > This code is all in include/private/trace-cmd-private.h and it doesn't > look like any of the existing functions (like tracecmd_get_trace_clock) > have man pages. Ah, my mistake. For some reason I thought these were going into include/trace-cmd/trace-cmd.h. Forget I mentioned it. Note, that the include/private/trace-cmd-private.h *is* a stepping stone to make them public. That is, everything in there will eventually move to trace-cmd.h as a public API, in which we will need a man page for. Since once it is in trace-cmd.h, it becomes a stable API (sketched in stone), we want to make sure that it's at its final stage before moving it there. APIs are hard :-p > > Seems like the trace-cmd man pages all describe the trace-cmd > executable? Not the Documentation/libtracecmd/ directory. > > For background, we're wanting to add these functions to this private > header file since we're pulling trace-input.c, trace-hooks.c, and > trace-util.c into our binary (along with libtracevent) to read events > from trace.dat files. Right now tracecmd_input struct members (like > uname) are local to trace-input.c and only externally visible via > tracecmd_print_uname(). Which isn't useful to a gui app, natch. Sounds like what you are doing is the perfect use case for us. We want libtracecmd to become available for all tools (not just guis). So, yes we are very interested in what you are doing. But we are being very conservative about this, because the code was done with a more specific use case, and to get the API correct, we need a broader view. As stated before, once we release a library with an API, it's going to be very difficult to change. I follow the Linux mantra of "Don't break user space" where I believe libraries should follow "Don't break users of the library". > > But yeah, please let me know how you'd like to proceed. Happy to > start a man page for trace-cmd-private.h but pretty sure I'm not the > best person to document *all* the current functions in this file. :) Forget I asked. I could have sworn you added to the public code. Anyway, it will be needed once we start adding those functions into the public header. > > > > +unsigned long long tracecmd_get_cpu_file_offset(struct > > > tracecmd_input *handle, int cpu) > > > > Should be "unsigned int cpu" > > There are a several existing functions that take an "int cpu" > parameter. > > Is this something where this new functions should use an unsigned int > and then update the others later, or perhaps add a check in the code > for negative cpu values and keep the "int cpu" parameter to stay > consistent with the others? I was more worried about a negative index into the array. It's fine to have int cpu, as long as you have a test of: if (cpu < 0) return -1; // or whatever I just didn't want out of bound accesses of arrays. > > > Anyway, welcome to our community Michael! > > Thanks for the feedback and warm welcome Steven - much appreciated! Looking forward to working with you some more. -- Steve
diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h index c58b09e..cd78cc9 100644 --- a/lib/trace-cmd/include/private/trace-cmd-private.h +++ b/lib/trace-cmd/include/private/trace-cmd-private.h @@ -90,6 +90,12 @@ bool tracecmd_get_quiet(struct tracecmd_output *handle); void tracecmd_set_out_clock(struct tracecmd_output *handle, const char *clock); const char *tracecmd_get_trace_clock(struct tracecmd_input *handle); +const char *tracecmd_get_cpustats(struct tracecmd_input *handle); +const char *tracecmd_get_uname(struct tracecmd_input *handle); +const char *tracecmd_get_version(struct tracecmd_input *handle); +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); + static inline int tracecmd_host_bigendian(void) { unsigned char str[] = { 0x1, 0x2, 0x3, 0x4 }; diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c index ac57bc4..99dbca2 100644 --- a/lib/trace-cmd/trace-input.c +++ b/lib/trace-cmd/trace-input.c @@ -4088,6 +4088,31 @@ const char *tracecmd_get_trace_clock(struct tracecmd_input *handle) return handle->trace_clock; } +const char *tracecmd_get_cpustats(struct tracecmd_input *handle) +{ + return handle->cpustats; +} + +const char *tracecmd_get_uname(struct tracecmd_input *handle) +{ + return handle->uname; +} + +const char *tracecmd_get_version(struct tracecmd_input *handle) +{ + return handle->version; +} + +unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu) +{ + return (cpu < handle->cpus) ? handle->cpu_data[cpu].file_offset : 0; +} + +unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu) +{ + return (cpu < handle->cpus) ? handle->cpu_data[cpu].file_size : 0; +} + /** * tracecmd_get_show_data_func - return the show data func * @handle: input handle for the trace.dat file
New APIs added to get trace cpustats, uname, version strings, and cpu file sizes and offsets. const char *tracecmd_get_cpustats(struct tracecmd_input *handle); const char *tracecmd_get_uname(struct tracecmd_input *handle); const char *tracecmd_get_version(struct tracecmd_input *handle); unsigned long long tracecmd_get_cpu_file_offset(struct tracecmd_input *handle, int cpu); unsigned long long tracecmd_get_cpu_file_size(struct tracecmd_input *handle, int cpu); Signed-off-by: Michael Sartain <mikesart@fastmail.com> --- .../include/private/trace-cmd-private.h | 6 +++++ lib/trace-cmd/trace-input.c | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+)