On Fri, 14 May 2021 16:45:51 +0300
Yordan Karadzhov <y.karadz@xxxxxxxxx> wrote:
> On 14.05.21 г. 16:31, Steven Rostedt wrote:
> > On Fri, 14 May 2021 15:18:25 +0300
> > "Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
> >
> >> When closing a "tep" data stream we destroy the "trace_seq" object.
> >> However, trace_seq_destroy() sets the buffer to "TRACE_SEQ_POISON"
> >> which is different from NULL.
> >>
> >> It is unfortunate that TRACE_SEQ_POISON is an internal definition
> >> of libtraceevent, so we have to redefine it here, but this can be
> >> fixed in the future.
> >
> > It's not unfortunate. It can change in the future without breaking API.
> >
> > Redefining it here is not robust, and if trace-seq decides to do something
> > different with that poison value, this will break, and it can't be blamed
> > on API (using internal knowledge to implement code is not protected by
> > being backward compatible).
> >
> > The correct solution is to NULL the buffer after calling destroy.
> >
> > if (seq.buffer) {
> > trace_seq_destroy(&seq);
> > seq.buffer = NULL;
> > }
>
> This was the first fix I did when I found the problem, but I don't like
> it because it looks like a hack the the user of the library is doing to
> trick the internal logic of the library.
It's not a hack. seq.buffer is exposed via the API (it's in the header) and
is allowed to be used (we use it all the time).
The trace_seq_destroy() function is only to clean up everything that
trace_seq_init() had done, and the seq is no longer valid, and the user is
free to do whatever they want with it afterward. Like set seq.buffer to
NULL.
This is a perfectly valid use case.
>
> Why not just moving the definition of TRACE_SEQ_POISON to the header or
> adding
Because TRACE_SEQ_POISON is an internal API that I never want to expose,
because I may even change it in the future. After destroy is called, the
trace_seq code is done. If you want to use the trace_seq again, you need to
call init.
Technically, if you want to do it prim and proper (but I don't actually
recommend this), you need to have another variable that keeps track of the
seq if it was allocated or not. That's not the responsibility of the
trace_seq API to do so. All the trace_seq API cares about is the time
trace_seq_init() is called till trace_seq_destroy() is called. Before or
after that, the seq is of no use to it.
You would need to have:
bool seq_is_init;
if (!seq_is_init) {
trace_seq_init(&seq);
seq_is_init = true;
}
and later
if (seq_is_init) {
trace_seq_destroy(&seq);
seq_is_init = false;
}
that's if you don't want to use seq.buffer == NULL to do that for you.
-- Steve
|