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@xxxxxxxxxxxx> 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
|