[PATCH 0/7] Final fixes before KS 2.0

[PATCH 0/7] Final fixes before KS 2.0

[Yordan Karadzhov (VMware)]

Yordan Karadzhov (VMware) (7):
  kernel-shark: Preserve markers when appending data
  kernel-shark: Preserve open graphs when appending data
  kernel-shark: Clear before loading new session
  kernel-shark: Better handling of plugins when appending data file
  kernel-shark: Do draw the combo point of the mark
  kernel-shark: Check if "trace_seq" was destroyed before using it
  kernel-shark: Quiet the warning printing from libtraceevent

 src/KsDualMarker.hpp    |  6 ++++++
 src/KsGLWidget.cpp      | 40 ++++++++++++++++++++++++++--------------
 src/KsGLWidget.hpp      |  4 +++-
 src/KsMainWindow.cpp    | 31 ++++++++++++++++++++++++++-----
 src/KsPlotTools.cpp     |  3 +++
 src/KsSession.cpp       |  1 +
 src/KsTraceGraph.cpp    | 10 +++++++---
 src/KsTraceGraph.hpp    |  2 +-
 src/KsUtils.cpp         |  4 ++--
 src/libkshark-tepdata.c | 16 +++++++++++++++-
 10 files changed, 90 insertions(+), 27 deletions(-)

-- 
2.27.0

[PATCH 1/7] kernel-shark: Preserve markers when appending data

[Yordan Karadzhov (VMware)]

When an entry is selected in the KernelShark GUI (using marker A and
marker B) we only keep track the index of this entry inside the array
of entries loaded at the moment of selecting. However, then a data
file is appended, the new entries are merged to this array and the
array is sorted. As a result the index of the marker can/will point
to completely different entry.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsDualMarker.hpp |  6 ++++++
 src/KsMainWindow.cpp | 24 ++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/KsDualMarker.hpp b/src/KsDualMarker.hpp
index 0dcaf93..39c0ce2 100644
--- a/src/KsDualMarker.hpp
+++ b/src/KsDualMarker.hpp
@@ -154,6 +154,12 @@ public:
 
 	void updateLabels();
 
+	/** Get the index inside the data array marker A points to. */
+	ssize_t markerAPos() {return markerA()._isSet ? markerA()._pos : -1;}
+
+	/** Get the index inside the data array marker B points to. */
+	ssize_t markerBPos() {return markerB()._isSet ? markerB()._pos : -1;}
+
 signals:
 	/**
 	 * This signal is emitted when the Table View has to switch the color
diff --git a/src/KsMainWindow.cpp b/src/KsMainWindow.cpp
index d0a434a..fa893ce 100644
--- a/src/KsMainWindow.cpp
+++ b/src/KsMainWindow.cpp
@@ -571,8 +571,15 @@ void KsMainWindow::markEntry(ssize_t row, DualMarkerState st)
 /** Select given kshark_entry with a given maker. */
 void KsMainWindow::markEntry(const kshark_entry *e, DualMarkerState st)
 {
-	ssize_t row = kshark_find_entry_by_time(e->ts, _data.rows(),
-						0, _data.size() - 1);
+	ssize_t row;
+
+	if (!e) {
+		_mState.getMarker(st).reset();
+		return;
+	}
+
+	row = kshark_find_entry_by_time(e->ts, _data.rows(),
+					0, _data.size() - 1);
 
 	markEntry(row, st);
 }
@@ -1341,7 +1348,20 @@ void KsMainWindow::loadDataFile(const QString& fileName)
 /** Append trace data for file. */
 void KsMainWindow::appendDataFile(const QString& fileName)
 {
+	kshark_entry *eMarkA(nullptr), *eMarkB(nullptr);
+	int rowA = _mState.markerAPos();
+	int rowB = _mState.markerBPos();
+
+	if (rowA >= 0)
+		eMarkA = _data.rows()[rowA];
+
+	if (rowB >= 0)
+		eMarkB = _data.rows()[rowB];
+
 	_load(fileName, true);
+
+	markEntry(eMarkA, DualMarkerState::A);
+	markEntry(eMarkB, DualMarkerState::B);
 }
 
 void KsMainWindow::_error(const QString &mesg,
-- 
2.27.0

[PATCH 2/7] kernel-shark: Preserve open graphs when appending data

[Yordan Karadzhov (VMware)]

If the user has already made some selection of graphs to be displayed
by the GUI, those particular graphs are likely to be important for him,
so it is better to keep this configuration after the data file is
appended. The opposite can be annoying for the user.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsGLWidget.cpp   | 40 ++++++++++++++++++++++++++--------------
 src/KsGLWidget.hpp   |  4 +++-
 src/KsMainWindow.cpp |  7 ++++---
 src/KsTraceGraph.cpp | 10 +++++++---
 src/KsTraceGraph.hpp |  2 +-
 src/KsUtils.cpp      |  1 +
 6 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/src/KsGLWidget.cpp b/src/KsGLWidget.cpp
index 9f8e43b..8aab7b3 100644
--- a/src/KsGLWidget.cpp
+++ b/src/KsGLWidget.cpp
@@ -409,25 +409,12 @@ void KsGLWidget::keyReleaseEvent(QKeyEvent *event)
  */
 #define KS_MAX_START_PLOTS 16
 
-/**
- * @brief Load and show trace data.
- *
- * @param data: Input location for the KsDataStore object.
- *	  KsDataStore::loadDataFile() must be called first.
- */
-void KsGLWidget::loadData(KsDataStore *data)
+void KsGLWidget::_defaultPlots(kshark_context *kshark_ctx)
 {
-	kshark_context *kshark_ctx(nullptr);
 	QVector<int> streamIds, plotVec;
 	uint64_t tMin, tMax;
 	int nCPUs, nBins;
 
-	if (!kshark_instance(&kshark_ctx) || !kshark_ctx->n_streams)
-		return;
-
-	loadColors();
-
-	_data = data;
 	_model.reset();
 	_streamPlots.clear();
 
@@ -463,6 +450,31 @@ void KsGLWidget::loadData(KsDataStore *data)
 	tMin = _data->rows()[0]->ts;
 	tMax = _data->rows()[_data->size() - 1]->ts;
 	ksmodel_set_bining(_model.histo(), nBins, tMin, tMax);
+}
+
+/**
+ * @brief Load and show trace data.
+ *
+ * @param data: Input location for the KsDataStore object.
+ *		KsDataStore::loadDataFile() must be called first.
+ * @param resetPlots: If true, all existing graphs are closed
+ *		      and a default configuration of graphs is displayed
+ *		      (all CPU plots). If false, the current set of graphs
+ *		      is preserved.
+ */
+void KsGLWidget::loadData(KsDataStore *data, bool resetPlots)
+{
+	kshark_context *kshark_ctx(nullptr);
+
+	if (!kshark_instance(&kshark_ctx) || !kshark_ctx->n_streams)
+		return;
+
+	loadColors();
+
+	_data = data;
+	if (resetPlots)
+		_defaultPlots(kshark_ctx);
+
 	_model.fill(_data);
 }
 
diff --git a/src/KsGLWidget.hpp b/src/KsGLWidget.hpp
index 629ae37..6a72a35 100644
--- a/src/KsGLWidget.hpp
+++ b/src/KsGLWidget.hpp
@@ -101,7 +101,7 @@ public:
 
 	void keyReleaseEvent(QKeyEvent *event);
 
-	void loadData(KsDataStore *data);
+	void loadData(KsDataStore *data, bool resetPlots);
 
 	void loadColors();
 
@@ -326,6 +326,8 @@ private:
 	int _getLastCPU(struct kshark_trace_histo *histo,
 			int bin, int sd, int pid);
 
+	void _defaultPlots(kshark_context *kshark_ctx);
+
 	void _deselect();
 
 	int _bin0Offset() {return _labelSize + 2 * _hMargin;}
diff --git a/src/KsMainWindow.cpp b/src/KsMainWindow.cpp
index fa893ce..1cbec55 100644
--- a/src/KsMainWindow.cpp
+++ b/src/KsMainWindow.cpp
@@ -1283,7 +1283,8 @@ void KsMainWindow::_load(const QString& fileName, bool append)
 	QApplication::processEvents();
 
 	_view.reset();
-	_graph.reset();
+	if (!append)
+		_graph.reset();
 
 	auto lamLoadJob = [&, this] () {
 		QVector<kshark_dpi *> v;
@@ -1333,7 +1334,7 @@ void KsMainWindow::_load(const QString& fileName, bool append)
 	_view.loadData(&_data);
 	pb.setValue(175);
 
-	_graph.loadData(&_data);
+	_graph.loadData(&_data, !append);
 	pb.setValue(195);
 }
 
@@ -1454,7 +1455,7 @@ void KsMainWindow::loadSession(const QString &fileName)
 	_view.loadData(&_data);
 	pb.setValue(155);
 
-	_graph.loadData(&_data);
+	_graph.loadData(&_data, true);
 	_filterSyncCBoxUpdate(kshark_ctx);
 	pb.setValue(175);
 
diff --git a/src/KsTraceGraph.cpp b/src/KsTraceGraph.cpp
index 1e976df..65e5a79 100644
--- a/src/KsTraceGraph.cpp
+++ b/src/KsTraceGraph.cpp
@@ -148,12 +148,16 @@ KsTraceGraph::KsTraceGraph(QWidget *parent)
  * @brief Load and show trace data.
  *
  * @param data: Input location for the KsDataStore object.
- *	  KsDataStore::loadDataFile() must be called first.
+ *	  	KsDataStore::loadDataFile() must be called first.
+ * @param resetPlots: If true, all existing graphs are closed
+ *		      and a default configuration of graphs is displayed
+ *		      (all CPU plots). If false, the current set of graphs
+ *		      is preserved.
  */
-void KsTraceGraph::loadData(KsDataStore *data)
+void KsTraceGraph::loadData(KsDataStore *data, bool resetPlots)
 {
 	_data = data;
-	_glWindow.loadData(data);
+	_glWindow.loadData(data, resetPlots);
 	updateGeom();
 }
 
diff --git a/src/KsTraceGraph.hpp b/src/KsTraceGraph.hpp
index 6e83f21..b1132e0 100644
--- a/src/KsTraceGraph.hpp
+++ b/src/KsTraceGraph.hpp
@@ -45,7 +45,7 @@ class KsTraceGraph : public KsWidgetsLib::KsDataWidget
 public:
 	explicit KsTraceGraph(QWidget *parent = nullptr);
 
-	void loadData(KsDataStore *data);
+	void loadData(KsDataStore *data, bool resetPlots);
 
 	void setMarkerSM(KsDualMarkerSM *m);
 
diff --git a/src/KsUtils.cpp b/src/KsUtils.cpp
index ec53267..757f49c 100644
--- a/src/KsUtils.cpp
+++ b/src/KsUtils.cpp
@@ -785,6 +785,7 @@ int KsDataStore::appendDataFile(const QString &file, int64_t offset)
 	_rows = mergedRows;
 
 	registerCPUCollections();
+	emit updateWidgets(this);
 
 	return sd;
 }
-- 
2.27.0

[PATCH 3/7] kernel-shark: Clear before loading new session

[Yordan Karadzhov (VMware)]

Make sure that all already loaded Data streams are properly closed,
before loading a new configuration from session description file.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsSession.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/KsSession.cpp b/src/KsSession.cpp
index 1e19a5f..b9edb3a 100644
--- a/src/KsSession.cpp
+++ b/src/KsSession.cpp
@@ -128,6 +128,7 @@ void KsSession::loadDataStreams(kshark_context *kshark_ctx,
 
 	data->unregisterCPUCollections();
 
+	kshark_close_all(kshark_ctx);
 	dataSize = kshark_import_all_dstreams(kshark_ctx,
 					      _config,
 					      data->rows_r());
-- 
2.27.0

[PATCH 4/7] kernel-shark: Better handling of plugins when appending data file

[Yordan Karadzhov (VMware)]

It may sound like a good idea to reinitialize all plugins for all
existing Data streams after a new stream is appended, but it isn't.
Such reset will re-initialize all Data containers open by the
plugins, hence it requires to do a complete reload of all previously
loaded data. Currently we reset the plugins but do not reload the
data, which is a bug. Because, reloading the data can be slow on
large data sets, I prefer to fix the bug, by eliminating the reload
of the plugins.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsUtils.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/KsUtils.cpp b/src/KsUtils.cpp
index 757f49c..3db8951 100644
--- a/src/KsUtils.cpp
+++ b/src/KsUtils.cpp
@@ -677,8 +677,7 @@ int KsDataStore::_openDataFile(kshark_context *kshark_ctx,
 
 	if (kshark_is_tep(kshark_ctx->stream[sd])) {
 		kshark_tep_init_all_buffers(kshark_ctx, sd);
-		for (int i = 0; i < kshark_ctx->n_streams; ++i)
-			kshark_tep_handle_plugins(kshark_ctx, i);
+		kshark_tep_handle_plugins(kshark_ctx, sd);
 	}
 
 	return sd;
-- 
2.27.0

[PATCH 5/7] kernel-shark: Do draw the combo point of the mark

[Yordan Karadzhov (VMware)]

The lines of the code that initialize and plot the combo point
are missing in the original commit.

Fixing: fc14e40 (kernel-shark: Add combo point to Mark)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsPlotTools.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/KsPlotTools.cpp b/src/KsPlotTools.cpp
index abef5f8..1d63a9b 100644
--- a/src/KsPlotTools.cpp
+++ b/src/KsPlotTools.cpp
@@ -680,6 +680,8 @@ Mark::Mark()
 	_cpu._size = 5.5f;
 	_task._color = Color(0, 255, 0);
 	_task._size = 5.5f;
+	_combo._color = Color(100, 150, 255);
+	_combo._size = 5.5f;
 }
 
 void Mark::_draw(const Color &col, float size) const
@@ -691,6 +693,7 @@ void Mark::_draw(const Color &col, float size) const
 
 	_cpu.draw();
 	_task.draw();
+	_combo.draw();
 }
 
 /**
-- 
2.27.0

[PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Yordan Karadzhov (VMware)]

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(-)

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;
-- 
2.27.0

[PATCH 7/7] kernel-shark: Quiet the warning printing from libtraceevent

[Yordan Karadzhov (VMware)]

All those warning messages are not really relevant for the users
of the GUI.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/libkshark-tepdata.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
index 4a84141..323383f 100644
--- a/src/libkshark-tepdata.c
+++ b/src/libkshark-tepdata.c
@@ -45,6 +45,14 @@ static bool init_thread_seq(void)
 	return seq.buffer != NULL;
 }
 
+//! @cond Doxygen_Suppress
+
+void tep_warning(const char *fmt, ...) {}
+
+void pr_stat(const char *fmt, ...) {}
+
+//! @endcond
+
 /** Structure for handling all unique attributes of the FTRACE data. */
 struct tepdata_handle {
 	/** Page event used to parse the page. */
-- 
2.27.0

Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Steven Rostedt]

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;

Re: [PATCH 7/7] kernel-shark: Quiet the warning printing from libtraceevent

[Steven Rostedt]

On Fri, 14 May 2021 15:18:26 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> All those warning messages are not really relevant for the users
> of the GUI.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  src/libkshark-tepdata.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
> index 4a84141..323383f 100644
> --- a/src/libkshark-tepdata.c
> +++ b/src/libkshark-tepdata.c
> @@ -45,6 +45,14 @@ static bool init_thread_seq(void)
>  	return seq.buffer != NULL;
>  }
>  
> +//! @cond Doxygen_Suppress
> +
> +void tep_warning(const char *fmt, ...) {}
> +
> +void pr_stat(const char *fmt, ...) {}
> +
> +//! @endcond
> +
>  /** Structure for handling all unique attributes of the FTRACE data. */
>  struct tepdata_handle {
>  	/** Page event used to parse the page. */

We released libtraceevent 1.3 that has the new logging by Tzvetomir. You
can set it to be quiet without the need to redefine these functions (which
have also been labeled as deprecated).

-- Steve

Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Yordan Karadzhov]

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;
> 

Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Steven Rostedt]

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

Re: [PATCH 6/7] kernel-shark: Check if "trace_seq" was destroyed before using it

[Yordan Karadzhov]

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.

Re: [PATCH 0/7] Final fixes before KS 2.0

[Steven Rostedt]

On Fri, 14 May 2021 15:18:19 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Yordan Karadzhov (VMware) (7):
>   kernel-shark: Preserve markers when appending data
>   kernel-shark: Preserve open graphs when appending data
>   kernel-shark: Clear before loading new session
>   kernel-shark: Better handling of plugins when appending data file
>   kernel-shark: Do draw the combo point of the mark

>   kernel-shark: Check if "trace_seq" was destroyed before using it

Besides the above patch, the rest look good.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

>   kernel-shark: Quiet the warning printing from libtraceevent
> 
>  src/KsDualMarker.hpp    |  6 ++++++
>  src/KsGLWidget.cpp      | 40 ++++++++++++++++++++++++++--------------
>  src/KsGLWidget.hpp      |  4 +++-
>  src/KsMainWindow.cpp    | 31 ++++++++++++++++++++++++++-----
>  src/KsPlotTools.cpp     |  3 +++
>  src/KsSession.cpp       |  1 +
>  src/KsTraceGraph.cpp    | 10 +++++++---
>  src/KsTraceGraph.hpp    |  2 +-
>  src/KsUtils.cpp         |  4 ++--
>  src/libkshark-tepdata.c | 16 +++++++++++++++-
>  10 files changed, 90 insertions(+), 27 deletions(-)
> 

Re: [PATCH 0/7] Final fixes before KS 2.0

[Steven Rostedt]

On Fri, 14 May 2021 15:18:19 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Yordan Karadzhov (VMware) (7):
>   kernel-shark: Preserve markers when appending data
>   kernel-shark: Preserve open graphs when appending data
>   kernel-shark: Clear before loading new session
>   kernel-shark: Better handling of plugins when appending data file
>   kernel-shark: Do draw the combo point of the mark
>   kernel-shark: Check if "trace_seq" was destroyed before using it
>   kernel-shark: Quiet the warning printing from libtraceevent
> 
>  

After applying and testing out this patch set, I realized that the appended
file does not have any of its CPUs plotted after it is loaded.

-- Steve

Re: [PATCH 0/7] Final fixes before KS 2.0

[Yordan Karadzhov]

On 14.05.21 г. 21:01, Steven Rostedt wrote:
> On Fri, 14 May 2021 15:18:19 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> Yordan Karadzhov (VMware) (7):
>>    kernel-shark: Preserve markers when appending data
>>    kernel-shark: Preserve open graphs when appending data
>>    kernel-shark: Clear before loading new session
>>    kernel-shark: Better handling of plugins when appending data file
>>    kernel-shark: Do draw the combo point of the mark
>>    kernel-shark: Check if "trace_seq" was destroyed before using it
>>    kernel-shark: Quiet the warning printing from libtraceevent
>>
>>   
> 
> After applying and testing out this patch set, I realized that the appended
> file does not have any of its CPUs plotted after it is loaded.

Yes, this is how it works right now. No new plots are added when you 
append trace file.

Do you think it will be to show all CPU plots from the new trace file?

Thanks!
Yordan

> 
> -- Steve
> 

Re: [PATCH 0/7] Final fixes before KS 2.0

[Steven Rostedt]

On Mon, 17 May 2021 13:22:34 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> On 14.05.21 г. 21:01, Steven Rostedt wrote:
> > On Fri, 14 May 2021 15:18:19 +0300
> > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> >   
> >> Yordan Karadzhov (VMware) (7):
> >>    kernel-shark: Preserve markers when appending data
> >>    kernel-shark: Preserve open graphs when appending data
> >>    kernel-shark: Clear before loading new session
> >>    kernel-shark: Better handling of plugins when appending data file
> >>    kernel-shark: Do draw the combo point of the mark
> >>    kernel-shark: Check if "trace_seq" was destroyed before using it
> >>    kernel-shark: Quiet the warning printing from libtraceevent
> >>
> >>     
> > 
> > After applying and testing out this patch set, I realized that the appended
> > file does not have any of its CPUs plotted after it is loaded.  
> 
> Yes, this is how it works right now. No new plots are added when you 
> append trace file.
> 
> Do you think it will be to show all CPU plots from the new trace file?
> 

Yeah, I think it is better to see something than nothing.

I don't think there's an issue with showing it. Although, if you could
automate the kvm combo to be selected by default on appending a file, then
you wouldn't need to show the cpu plots of the guest.

-- Steve

Re: [PATCH 0/7] Final fixes before KS 2.0

[Yordan Karadzhov]

On 17.05.21 г. 15:54, Steven Rostedt wrote:
> On Mon, 17 May 2021 13:22:34 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>> On 14.05.21 г. 21:01, Steven Rostedt wrote:
>>> On Fri, 14 May 2021 15:18:19 +0300
>>> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
>>>    
>>>> Yordan Karadzhov (VMware) (7):
>>>>     kernel-shark: Preserve markers when appending data
>>>>     kernel-shark: Preserve open graphs when appending data
>>>>     kernel-shark: Clear before loading new session
>>>>     kernel-shark: Better handling of plugins when appending data file
>>>>     kernel-shark: Do draw the combo point of the mark
>>>>     kernel-shark: Check if "trace_seq" was destroyed before using it
>>>>     kernel-shark: Quiet the warning printing from libtraceevent
>>>>
>>>>      
>>>
>>> After applying and testing out this patch set, I realized that the appended
>>> file does not have any of its CPUs plotted after it is loaded.
>>
>> Yes, this is how it works right now. No new plots are added when you
>> append trace file.
>>
>> Do you think it will be to show all CPU plots from the new trace file?
>>
> 
> Yeah, I think it is better to see something than nothing.

OK, I will send v2 that is doing this. It will be a trivial change.


> 
> I don't think there's an issue with showing it. Although, if you could
> automate the kvm combo to be selected by default on appending a file, then
> you wouldn't need to show the cpu plots of the guest.

This can be done as well but it may be a bit tricky. We should probably 
also provide a command line option for selecting combo plots.

But let's release v2.0 now and keep this feature for v2.1, together with 
the other features you requested on Bugzilla.

Thanks!
Yordan

> 
> -- Steve
>