Re: [PATCH] libtracefs: Add new API for open trace marker file

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

Hi Steven,

On Fri, Feb 19, 2021 at 4:26 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 19 Feb 2021 07:53:53 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > Added new API for opening trace_marker file of given instance:
> >    tracefs_trace_marker_get_fd();
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >
>
> As I wrote the perf-trace.c program, I was thinking what we really should
> have is the following API. We can keep this API, but what would be nice is:
>
>
>         int tracefs_print_init(struct tracefs_instance *instance);
>
>         int tracefs_print(struct tracefs_instance *instance,
>                          const char *fmt, ...);
>
>         int tracefs_vprint(struct tracefs_instance *instance,
>                            const char *fmt, va_list ap);
>
>         void tracefs_print_reset(struct tracefs_instance *instance);
>
> Where tracefs_print_init() will open the trace_marker for that instance
> (NULL being the top level), and storing it in the instance structure.

You mean to hold the marker fd in the tracefs_instance structure ?
I like such approach, to hold some library specific context in that
structure, internally and not visible from the user. In that case we do
not need tracefs_print_init() at all, the first call to some tracefs_print
API will open the file. But that will make the APIs not thread safe, is
it OK marker fd to be used from multiple threads at the same time ?

>
> tracefs_print() and tracefs_vprint() will check if the trace_marker file
> has already been opened (tracefs_print_init() was previously called), and
> if not, it will open it and keep it open. Then it will write to the
> trace_marker file the passed in print data after formatting it (see my
> trace_print in perf-trace.c).
>
> The tracefs_print_reset() will simply close the trace_marker file if it was
> previously opened, note, so will any of the destructors of the instance.
>
> We could also have:
>
>         int tracefs_raw_print_init(struct tracefs_instance *instance);
>
>         int tracefs_raw_print(struct tracefs_instance *instance,
>                           void *data, int len);
>
>         void tracefs_raw_print_reset(struct tracefs_instance *instance);
>
>
> That is the same, but instead of writing string data to the trace_marker,
> it would write in memory into trace_marker_raw.

I'm afraid that having too many APIs with sort of overlapping functionality
could make the library hard to use ? Actually the proposed new API by this
patch, tracefs_trace_marker_get_fd(), already duplicates the existing
tracefs_instance_file_open() API.

>
> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center