archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <>
To: Yordan Karadzhov <>
Subject: Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it
Date: Fri, 14 May 2021 09:57:43 -0400	[thread overview]
Message-ID: <20210514095743.4f265de0@gandalf.local.home> (raw)
In-Reply-To: <>

On Fri, 14 May 2021 16:45:51 +0300
Yordan Karadzhov <> wrote:

> On 14.05.21 г. 16:31, Steven Rostedt wrote:
> > On Fri, 14 May 2021 15:18:25 +0300
> > "Yordan Karadzhov (VMware)" <> 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

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) {
		seq_is_init = true;

and later

	if (seq_is_init) {
		seq_is_init = false;

that's if you don't want to use seq.buffer == NULL to do that for you.

-- Steve

  reply	other threads:[~2021-05-14 13:57 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 12:18 [PATCH 0/7] Final fixes before KS 2.0 Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 1/7] kernel-shark: Preserve markers when appending data Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 2/7] kernel-shark: Preserve open graphs " Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 3/7] kernel-shark: Clear before loading new session Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 4/7] kernel-shark: Better handling of plugins when appending data file Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 5/7] kernel-shark: Do draw the combo point of the mark Yordan Karadzhov (VMware)
2021-05-14 12:18 ` [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it Yordan Karadzhov (VMware)
2021-05-14 13:31   ` Steven Rostedt
2021-05-14 13:45     ` Yordan Karadzhov
2021-05-14 13:57       ` Steven Rostedt [this message]
2021-05-14 14:01         ` Yordan Karadzhov
2021-05-14 12:18 ` [PATCH 7/7] kernel-shark: Quiet the warning printing from libtraceevent Yordan Karadzhov (VMware)
2021-05-14 13:33   ` Steven Rostedt
2021-05-14 17:45 ` [PATCH 0/7] Final fixes before KS 2.0 Steven Rostedt
2021-05-14 18:01 ` Steven Rostedt
2021-05-17 10:22   ` Yordan Karadzhov
2021-05-17 12:54     ` Steven Rostedt
2021-05-17 13:04       ` Yordan Karadzhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210514095743.4f265de0@gandalf.local.home \ \ \ \
    --subject='Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).