| Message ID | 20210514121826.161749-7-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Final fixes before KS 2.0 | expand |
On Fri, 14 May 2021 15:18:25 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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; } Just like you would do with a pointer you free but may use NULL as a value you are checking. -- Steve > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/libkshark-tepdata.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c > index bc5babb..4a84141 100644 > --- a/src/libkshark-tepdata.c > +++ b/src/libkshark-tepdata.c > @@ -29,11 +29,17 @@ > #include "libkshark-plugin.h" > #include "libkshark-tepdata.h" > > +/** > + * The TEP_SEQ_POISON is to catch the use of > + * a trace_seq structure after it was destroyed. > + */ > +#define TEP_SEQ_POISON ((void *)0xdeadbeef) > + > static __thread struct trace_seq seq; > > static bool init_thread_seq(void) > { > - if (!seq.buffer) > + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) > trace_seq_init(&seq); > > return seq.buffer != NULL;
On 14.05.21 г. 16:31, Steven Rostedt wrote: > On Fri, 14 May 2021 15:18:25 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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. Why not just moving the definition of TRACE_SEQ_POISON to the header or adding book trace_seq_is_destroyed() After this we can remove the clone from here. Thanks! Yordan > > Just like you would do with a pointer you free but may use NULL as a value > you are checking. > > -- Steve > > >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> src/libkshark-tepdata.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c >> index bc5babb..4a84141 100644 >> --- a/src/libkshark-tepdata.c >> +++ b/src/libkshark-tepdata.c >> @@ -29,11 +29,17 @@ >> #include "libkshark-plugin.h" >> #include "libkshark-tepdata.h" >> >> +/** >> + * The TEP_SEQ_POISON is to catch the use of >> + * a trace_seq structure after it was destroyed. >> + */ >> +#define TEP_SEQ_POISON ((void *)0xdeadbeef) >> + >> static __thread struct trace_seq seq; >> >> static bool init_thread_seq(void) >> { >> - if (!seq.buffer) >> + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) >> trace_seq_init(&seq); >> >> return seq.buffer != NULL; >
On Fri, 14 May 2021 16:45:51 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > On 14.05.21 г. 16:31, Steven Rostedt wrote: > > On Fri, 14 May 2021 15:18:25 +0300 > > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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
On 14.05.21 г. 16:57, Steven Rostedt wrote: > 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. OK, thanks! Y.
diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c index bc5babb..4a84141 100644 --- a/src/libkshark-tepdata.c +++ b/src/libkshark-tepdata.c @@ -29,11 +29,17 @@ #include "libkshark-plugin.h" #include "libkshark-tepdata.h" +/** + * The TEP_SEQ_POISON is to catch the use of + * a trace_seq structure after it was destroyed. + */ +#define TEP_SEQ_POISON ((void *)0xdeadbeef) + static __thread struct trace_seq seq; static bool init_thread_seq(void) { - if (!seq.buffer) + if (!seq.buffer || seq.buffer == TEP_SEQ_POISON) trace_seq_init(&seq); return seq.buffer != NULL;
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. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/libkshark-tepdata.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)