[PATCH 00/11] Small modifications and bug fixes toward KS 1.0

[PATCH 00/11] Small modifications and bug fixes toward KS 1.0

[Yordan Karadzhov]

This series of patches contains various independent modifications and
bug fixes needed before releasing KernelShark 1.0.

Yordan Karadzhov (11):
  kernel-shark-qt: Protect all calls of tep_read_number_field()
  kernel-shark-qt: Fix the returned error value of
    kshark_get_event_id_easy()
  kernel-shark-qt: Avoid race condition in kshark_get_event_name_easy()
  kernel-shark-qt: Optimize the search in the text data
  kernel-shark-qt: Add iterator index to the search panel
  kernel-shark-qt: Update search iterator when marker is changed
  kernel-shark-qt: Optimize the search in a case of a small data-set
  kernel-shark qt: No error when Record authentication dialog is closed
  kernel-shark-qt: Remove all system=ftrace events from Record dialog
  kernel-shark-qt: Updata Event filter mask when applaing filters to
    Graph
  kernel-shark-qt: Reprocess all CPU collections when the filtering
    changes

 kernel-shark-qt/src/KsCaptureDialog.cpp    |   1 +
 kernel-shark-qt/src/KsMainWindow.cpp       |   8 ++
 kernel-shark-qt/src/KsModels.cpp           |  28 ++--
 kernel-shark-qt/src/KsModels.hpp           |   4 +-
 kernel-shark-qt/src/KsTraceViewer.cpp      | 152 +++++++++++++++------
 kernel-shark-qt/src/KsTraceViewer.hpp      |   9 +-
 kernel-shark-qt/src/KsUtils.cpp            |   6 +
 kernel-shark-qt/src/KsWidgetsLib.cpp       |  10 ++
 kernel-shark-qt/src/KsWidgetsLib.hpp       |   2 +
 kernel-shark-qt/src/libkshark.c            |  15 +-
 kernel-shark-qt/src/plugins/sched_events.c |  52 ++++---
 11 files changed, 204 insertions(+), 83 deletions(-)

-- 
2.17.1

[PATCH 01/11] kernel-shark-qt: protect all calls of tep_read_number_field()

[Yordan Karadzhov]

tep_read_number_field() is being used to retrieve the value of a data
field and this value has being used without checking if the function
succeeded. This is a potential bug because tep_read_number_field() may
fаil and in such a case the retrieved field value will be arbitrary.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/plugins/sched_events.c | 52 +++++++++++++---------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/kernel-shark-qt/src/plugins/sched_events.c b/kernel-shark-qt/src/plugins/sched_events.c
index 1851569..c22e198 100644
--- a/kernel-shark-qt/src/plugins/sched_events.c
+++ b/kernel-shark-qt/src/plugins/sched_events.c
@@ -97,10 +97,12 @@ int plugin_get_next_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
 
-	tep_read_number_field(plugin_ctx->sched_switch_next_field,
-			      record->data, &val);
-	return val;
+	ret = tep_read_number_field(plugin_ctx->sched_switch_next_field,
+				    record->data, &val);
+
+	return (ret == 0) ? val : ret;
 }
 
 /**
@@ -113,10 +115,12 @@ int plugin_get_rec_wakeup_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
+
+	ret = tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
+				    record->data, &val);
 
-	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
-			      record->data, &val);
-	return val;
+	return (ret == 0) ? val : ret;
 }
 
 static void plugin_register_command(struct kshark_context *kshark_ctx,
@@ -145,11 +149,12 @@ static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
 
-	tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
-				 record->data, &val);
+	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
+				    record->data, &val);
 
-	return val;
+	return (ret == 0) ? val : ret;
 }
 
 /**
@@ -170,7 +175,7 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 	struct plugin_sched_context *plugin_ctx;
 	struct tep_record *record = NULL;
 	unsigned long long val;
-	int wakeup_pid = -1;
+	int ret, wakeup_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 	if (!plugin_ctx)
@@ -181,10 +186,10 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 		record = kshark_read_at(kshark_ctx, e->offset);
 
 		/* We only want those that actually woke up the task. */
-		tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
+					    record->data, &val);
 
-		if (val)
+		if (ret == 0 && val)
 			wakeup_pid = plugin_get_rec_wakeup_pid(record);
 	}
 
@@ -193,10 +198,10 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 		record = kshark_read_at(kshark_ctx, e->offset);
 
 		/* We only want those that actually woke up the task. */
-		tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
+					    record->data, &val);
 
-		if (val)
+		if (ret == 0 && val)
 			wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
 	}
 
@@ -224,7 +229,7 @@ bool plugin_switch_match_rec_pid(struct kshark_context *kshark_ctx,
 {
 	struct plugin_sched_context *plugin_ctx;
 	unsigned long long val;
-	int switch_pid = -1;
+	int ret, switch_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 
@@ -233,10 +238,10 @@ bool plugin_switch_match_rec_pid(struct kshark_context *kshark_ctx,
 		struct tep_record *record;
 
 		record = kshark_read_at(kshark_ctx, e->offset);
-		tep_read_number_field(plugin_ctx->sched_switch_prev_state_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_switch_prev_state_field,
+					    record->data, &val);
 
-		if (!(val & 0x7f))
+		if (ret == 0 && !(val & 0x7f))
 			switch_pid = tep_data_pid(plugin_ctx->pevent, record);
 
 		free_record(record);
@@ -278,8 +283,11 @@ static void plugin_sched_action(struct kshark_context *kshark_ctx,
 				struct tep_record *rec,
 				struct kshark_entry *entry)
 {
-	entry->pid = plugin_get_next_pid(rec);
-	plugin_register_command(kshark_ctx, rec, entry->pid);
+	int pid = plugin_get_next_pid(rec);
+	if (pid >= 0) {
+		entry->pid = pid;
+		plugin_register_command(kshark_ctx, rec, entry->pid);
+	}
 }
 
 static int plugin_sched_init(struct kshark_context *kshark_ctx)
-- 
2.17.1

[PATCH 01/11] kernel-shark-qt: Protect all calls of tep_read_number_field()

[Yordan Karadzhov]

tep_read_number_field() is being used to retrieve the value of a data
field and this value has being used without checking if the function
succeeded. This is a potential bug because tep_read_number_field() may
fail and in such a case the retrieved field value will be arbitrary.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/plugins/sched_events.c | 52 +++++++++++++---------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/kernel-shark-qt/src/plugins/sched_events.c b/kernel-shark-qt/src/plugins/sched_events.c
index 1851569..c22e198 100644
--- a/kernel-shark-qt/src/plugins/sched_events.c
+++ b/kernel-shark-qt/src/plugins/sched_events.c
@@ -97,10 +97,12 @@ int plugin_get_next_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
 
-	tep_read_number_field(plugin_ctx->sched_switch_next_field,
-			      record->data, &val);
-	return val;
+	ret = tep_read_number_field(plugin_ctx->sched_switch_next_field,
+				    record->data, &val);
+
+	return (ret == 0) ? val : ret;
 }
 
 /**
@@ -113,10 +115,12 @@ int plugin_get_rec_wakeup_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
+
+	ret = tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
+				    record->data, &val);
 
-	tep_read_number_field(plugin_ctx->sched_wakeup_pid_field,
-			      record->data, &val);
-	return val;
+	return (ret == 0) ? val : ret;
 }
 
 static void plugin_register_command(struct kshark_context *kshark_ctx,
@@ -145,11 +149,12 @@ static int plugin_get_rec_wakeup_new_pid(struct tep_record *record)
 	struct plugin_sched_context *plugin_ctx =
 		plugin_sched_context_handler;
 	unsigned long long val;
+	int ret;
 
-	tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
-				 record->data, &val);
+	ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field,
+				    record->data, &val);
 
-	return val;
+	return (ret == 0) ? val : ret;
 }
 
 /**
@@ -170,7 +175,7 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 	struct plugin_sched_context *plugin_ctx;
 	struct tep_record *record = NULL;
 	unsigned long long val;
-	int wakeup_pid = -1;
+	int ret, wakeup_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 	if (!plugin_ctx)
@@ -181,10 +186,10 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 		record = kshark_read_at(kshark_ctx, e->offset);
 
 		/* We only want those that actually woke up the task. */
-		tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_wakeup_success_field,
+					    record->data, &val);
 
-		if (val)
+		if (ret == 0 && val)
 			wakeup_pid = plugin_get_rec_wakeup_pid(record);
 	}
 
@@ -193,10 +198,10 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx,
 		record = kshark_read_at(kshark_ctx, e->offset);
 
 		/* We only want those that actually woke up the task. */
-		tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field,
+					    record->data, &val);
 
-		if (val)
+		if (ret == 0 && val)
 			wakeup_pid = plugin_get_rec_wakeup_new_pid(record);
 	}
 
@@ -224,7 +229,7 @@ bool plugin_switch_match_rec_pid(struct kshark_context *kshark_ctx,
 {
 	struct plugin_sched_context *plugin_ctx;
 	unsigned long long val;
-	int switch_pid = -1;
+	int ret, switch_pid = -1;
 
 	plugin_ctx = plugin_sched_context_handler;
 
@@ -233,10 +238,10 @@ bool plugin_switch_match_rec_pid(struct kshark_context *kshark_ctx,
 		struct tep_record *record;
 
 		record = kshark_read_at(kshark_ctx, e->offset);
-		tep_read_number_field(plugin_ctx->sched_switch_prev_state_field,
-				      record->data, &val);
+		ret = tep_read_number_field(plugin_ctx->sched_switch_prev_state_field,
+					    record->data, &val);
 
-		if (!(val & 0x7f))
+		if (ret == 0 && !(val & 0x7f))
 			switch_pid = tep_data_pid(plugin_ctx->pevent, record);
 
 		free_record(record);
@@ -278,8 +283,11 @@ static void plugin_sched_action(struct kshark_context *kshark_ctx,
 				struct tep_record *rec,
 				struct kshark_entry *entry)
 {
-	entry->pid = plugin_get_next_pid(rec);
-	plugin_register_command(kshark_ctx, rec, entry->pid);
+	int pid = plugin_get_next_pid(rec);
+	if (pid >= 0) {
+		entry->pid = pid;
+		plugin_register_command(kshark_ctx, rec, entry->pid);
+	}
 }
 
 static int plugin_sched_init(struct kshark_context *kshark_ctx)
-- 
2.17.1

[PATCH 02/11] kernel-shark-qt: Fix the returned error value of kshark_get_event_id_easy()

[Yordan Karadzhov]

This patch fixes a simple bug in kshark_get_event_id_easy().
All returned error codes must be negative.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/libkshark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
index 1a39968..fa85171 100644
--- a/kernel-shark-qt/src/libkshark.c
+++ b/kernel-shark-qt/src/libkshark.c
@@ -1111,7 +1111,7 @@ int kshark_get_event_id_easy(struct kshark_entry *entry)
 		free_record(data);
 	}
 
-	return (event_id == -1)? EFAULT : event_id;
+	return (event_id == -1)? -EFAULT : event_id;
 }
 
 /**
@@ -1133,7 +1133,7 @@ const char *kshark_get_event_name_easy(struct kshark_entry *entry)
 	struct tep_event_format *event;
 
 	int event_id = kshark_get_event_id_easy(entry);
-	if (event_id == EFAULT)
+	if (event_id == -EFAULT)
 		return NULL;
 
 	kshark_instance(&kshark_ctx);
-- 
2.17.1

[PATCH 03/11] kernel-shark-qt: Avoid race condition in kshark_get_event_name_easy()

[Yordan Karadzhov]

Calling tep_data_event_from_type() is not thread-safe (potential
race condition). This patch protects the access to it by using the
mutex associated with the input file.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/libkshark.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
index fa85171..89ae769 100644
--- a/kernel-shark-qt/src/libkshark.c
+++ b/kernel-shark-qt/src/libkshark.c
@@ -1147,7 +1147,18 @@ const char *kshark_get_event_name_easy(struct kshark_entry *entry)
 		}
 	}
 
+	/*
+	 * Calling tep_data_event_from_type() is not thread-safe (potential
+	 * race condition).
+	 * TODO: See if we can add a thread-safe version of the
+	 * function. For the time being use a mutex to protect the access.
+	 */
+	pthread_mutex_lock(&kshark_ctx->input_mutex);
+
 	event = tep_data_event_from_type(kshark_ctx->pevent, event_id);
+
+	pthread_mutex_unlock(&kshark_ctx->input_mutex);
+
 	if (event)
 		return event->name;
 
-- 
2.17.1

[PATCH 04/11] kernel-shark-qt: Optimize the search in the text data

[Yordan Karadzhov]

This patch aims to optimize the search by doing two small
modifications:

It changes the type of the matching function used when searching.
This will eliminate the call of the QString copy constructor.

It also adds an intermediate step in the conversion of the tracing
data into the format expected by the QTableView widget (QVariant).
By having this intermediate step we avoid the unnecessary conversion
QString -> QVariant -> QString when searching.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsModels.cpp      | 28 ++++++++++++++++-----------
 kernel-shark-qt/src/KsModels.hpp      |  4 +++-
 kernel-shark-qt/src/KsTraceViewer.cpp |  6 +++---
 kernel-shark-qt/src/KsTraceViewer.hpp |  3 ---
 4 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/kernel-shark-qt/src/KsModels.cpp b/kernel-shark-qt/src/KsModels.cpp
index 5eb1646..d67ee62 100644
--- a/kernel-shark-qt/src/KsModels.cpp
+++ b/kernel-shark-qt/src/KsModels.cpp
@@ -59,8 +59,7 @@ void KsFilterProxyModel::_search(int column,
 {
 	int row, nRows(last - first + 1);
 	int pbCount(1);
-	QVariant item;
-	QString text;
+	QString item;
 
 	_searchProgress = 0;
 
@@ -76,10 +75,9 @@ void KsFilterProxyModel::_search(int column,
 		 * of the row number in the base model.
 		 */
 		row = mapRowFromSource(r);
-		item = _source->getValue(column, row);
-		if (cond(searchText, item.toString())) {
+		item = _source->getValueStr(column, row);
+		if (cond(searchText, item))
 			matchList->append(row);
-		}
 
 		if (_searchStop) {
 			_searchStop = false;
@@ -156,7 +154,6 @@ QList<int> KsFilterProxyModel::searchMap(int column,
 					 bool notify)
 {
 	QList<int> matchList;
-	qInfo() << "searchMap" << first << last;
 	_search(column, searchText, cond, &matchList, first, last,
 		nullptr, nullptr, notify);
 
@@ -203,15 +200,17 @@ QVariant KsViewModel::data(const QModelIndex &index, int role) const
 	return {};
 }
 
-/** Get the data stored in a given cell of the table. */
-QVariant KsViewModel::getValue(int column, int row) const
+/** Get the string data stored in a given cell of the table. */
+QString KsViewModel::getValueStr(int column, int row) const
 {
+	int pid;
+
 	switch (column) {
 		case TRACE_VIEW_COL_INDEX :
-			return row;
+			return QString("%1").arg(row);
 
 		case TRACE_VIEW_COL_CPU:
-			return _data[row]->cpu;
+			return QString("%1").arg(_data[row]->cpu);
 
 		case TRACE_VIEW_COL_TS:
 			return KsUtils::Ts2String(_data[row]->ts, 6);
@@ -220,7 +219,8 @@ QVariant KsViewModel::getValue(int column, int row) const
 			return kshark_get_task_easy(_data[row]);
 
 		case TRACE_VIEW_COL_PID:
-			return kshark_get_pid_easy(_data[row]);
+			pid = kshark_get_pid_easy(_data[row]);
+			return QString("%1").arg(pid);
 
 		case TRACE_VIEW_COL_LAT:
 			return kshark_get_latency_easy(_data[row]);
@@ -236,6 +236,12 @@ QVariant KsViewModel::getValue(int column, int row) const
 	}
 }
 
+/** Get the data stored in a given cell of the table. */
+QVariant KsViewModel::getValue(int column, int row) const
+{
+	return getValueStr(column, row);
+}
+
 /**
  * Get the header of a column. This is an implementation of the pure virtual
  * method of the abstract model class.
diff --git a/kernel-shark-qt/src/KsModels.hpp b/kernel-shark-qt/src/KsModels.hpp
index 00b20b2..b66c259 100644
--- a/kernel-shark-qt/src/KsModels.hpp
+++ b/kernel-shark-qt/src/KsModels.hpp
@@ -28,7 +28,7 @@
 #include "libkshark-model.h"
 
 /** Matching condition function type. To be user for searching. */
-typedef bool (*condition_func)(QString, QString);
+typedef bool (*condition_func)(const QString &, const QString &);
 
 enum class DualMarkerState;
 
@@ -81,6 +81,8 @@ public:
 	/** Get the list of column's headers. */
 	QStringList header() const {return _header;}
 
+	QString getValueStr(int column, int row) const;
+
 	QVariant getValue(int column, int row) const;
 
 	size_t search(int column,
diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index 7f12365..e92dd83 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -275,17 +275,17 @@ void KsTraceViewer::_graphFollowsChanged(int state)
 		emit select(*_it); // Send a signal to the Graph widget.
 }
 
-static bool notHaveCond(QString searchText, QString itemText)
+static bool notHaveCond(const QString &searchText, const QString &itemText)
 {
 	return !itemText.contains(searchText, Qt::CaseInsensitive);
 }
 
-static bool containsCond(QString searchText, QString itemText)
+static bool containsCond(const QString &searchText, const QString &itemText)
 {
 	return itemText.contains(searchText, Qt::CaseInsensitive);
 }
 
-static bool matchCond(QString searchText, QString itemText)
+static bool matchCond(const QString &searchText, const QString &itemText)
 {
 	return (itemText.compare(searchText, Qt::CaseInsensitive) == 0);
 }
diff --git a/kernel-shark-qt/src/KsTraceViewer.hpp b/kernel-shark-qt/src/KsTraceViewer.hpp
index 2a85e4f..de8af97 100644
--- a/kernel-shark-qt/src/KsTraceViewer.hpp
+++ b/kernel-shark-qt/src/KsTraceViewer.hpp
@@ -20,9 +20,6 @@
 #include "KsModels.hpp"
 #include "KsDualMarker.hpp"
 
-/** Matching condition function type. To be user for searchong. */
-typedef bool (*condition_func)(QString, QString);
-
 /**
  * The KsTraceViewer class provides a widget for browsing in the trace data
  * shown in a text form.
-- 
2.17.1

[PATCH 05/11] kernel-shark-qt: Add iterator index to the search panel

[Yordan Karadzhov]

The search panel displays the index inside the list of search
matching entries.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsTraceViewer.cpp | 17 +++++++++++++++--
 kernel-shark-qt/src/KsTraceViewer.hpp |  2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index e92dd83..3df4a5d 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -362,6 +362,8 @@ void KsTraceViewer::_next()
 		if (_graphFollows)
 			emit select(*_it); // Send a signal to the Graph widget.
 	}
+
+	_updateSearchCount();
 }
 
 void KsTraceViewer::_prev()
@@ -385,6 +387,16 @@ void KsTraceViewer::_prev()
 		if (_graphFollows)
 			emit select(*_it); // Send a signal to the Graph widget.
 	}
+
+	_updateSearchCount();
+}
+
+void KsTraceViewer::_updateSearchCount()
+{
+	int index(_it - _matchList.begin());
+	int total(_matchList.count());
+
+	_searchCountLabel.setText(QString(" %1 / %2").arg(index).arg(total));
 }
 
 void KsTraceViewer::_searchStop()
@@ -571,7 +583,6 @@ size_t KsTraceViewer::_searchItems(int column,
 	count = _matchList.count();
 
 	_pbAction->setVisible(false);
-	_searchCountLabel.setText(QString(" %1").arg(count));
 	_searchDone = true;
 
 	if (count == 0) // No items have been found. Do nothing.
@@ -605,6 +616,8 @@ size_t KsTraceViewer::_searchItems(int column,
 		_it = _matchList.begin();
 	}
 
+	_updateSearchCount();
+
 	return count;
 }
 
@@ -645,7 +658,7 @@ void KsTraceViewer::_searchItemsMapReduce(int column,
 	for (int r = 1; r < nThreads; ++r)
 		maps.push_back(std::async(lamSearchMap, ranges[r], false));
 
-	while (_proxyModel.searchProgress() < KS_PROGRESS_BAR_MAX- nThreads) {
+	while (_proxyModel.searchProgress() < KS_PROGRESS_BAR_MAX - nThreads) {
 		std::unique_lock<std::mutex> lk(_proxyModel._mutex);
 		_proxyModel._pbCond.wait(lk);
 		_searchProgBar.setValue(_proxyModel.searchProgress());
diff --git a/kernel-shark-qt/src/KsTraceViewer.hpp b/kernel-shark-qt/src/KsTraceViewer.hpp
index de8af97..19371de 100644
--- a/kernel-shark-qt/src/KsTraceViewer.hpp
+++ b/kernel-shark-qt/src/KsTraceViewer.hpp
@@ -132,6 +132,8 @@ private:
 
 	void _prev();
 
+	void _updateSearchCount();
+
 	void _searchStop();
 
 	void _clicked(const QModelIndex& i);
-- 
2.17.1

[PATCH 06/11] kernel-shark-qt: Update search iterator when marker is changed

[Yordan Karadzhov]

When the Dual marker changes it active state (between A and B) or
its selected row, the iterator over the list of search matching entries
has to be updated such that it points to the selected entry. If the
selected entry do not belong to the matching list, the iterator will
point to the first matching entry after the selected one.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsTraceViewer.cpp | 114 +++++++++++++++++---------
 kernel-shark-qt/src/KsTraceViewer.hpp |   4 +
 2 files changed, 80 insertions(+), 38 deletions(-)

diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index 3df4a5d..1f96234 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -350,10 +350,21 @@ void KsTraceViewer::_next()
 	}
 
 	if (!_matchList.empty()) { // Items have been found.
-		++_it; // Move the iterator.
-		if (_it == _matchList.end() ) {
-			// This is the last item of the list. Go back to the beginning.
-			_it = _matchList.begin();
+		int row = _getSelectedDataRow();
+		/*
+		 * The iterator can only be at the selected row or if the
+		 * selected row is not a match at the first matching row after
+		 * the selected one.
+		 */
+		if (*_it == row) {
+			++_it; // Move the iterator.
+			if (_it == _matchList.end() ) {
+				/*
+				 * This is the last item of the list.
+				 * Go back to the beginning.
+				 */
+				_it = _matchList.begin();
+			}
 		}
 
 		// Select the row of the item.
@@ -407,14 +418,17 @@ void KsTraceViewer::_searchStop()
 
 void KsTraceViewer::_clicked(const QModelIndex& i)
 {
-	if (_graphFollows) {
-		/*
-		 * Use the index of the proxy model to retrieve the value
-		 * of the row number in the base model.
-		 */
-		size_t row = _proxyModel.mapRowFromSource(i.row());
+	/*
+	 * Use the index of the proxy model to retrieve the value
+	 * of the row number in the base model.
+	 */
+	size_t row = _proxyModel.mapRowFromSource(i.row());
+
+	_setSearchIterator(row);
+	_updateSearchCount();
+
+	if (_graphFollows)
 		emit select(row); // Send a signal to the Graph widget.
-	}
 }
 
 /** Make a given row of the table visible. */
@@ -454,6 +468,8 @@ void KsTraceViewer::deselect()
 /** Switch the Dual marker. */
 void KsTraceViewer::markSwitch()
 {
+	int row;
+
 	/* The state of the Dual marker has changed. Get the new active marker. */
 	DualMarkerState state = _mState->getState();
 
@@ -495,6 +511,12 @@ void KsTraceViewer::markSwitch()
 	} else {
 		_view.clearSelection();
 	}
+
+	row = _getSelectedDataRow();
+	if (row >= 0) {
+		_setSearchIterator(row);
+		_updateSearchCount();
+	}
 }
 
 /**
@@ -529,12 +551,9 @@ void KsTraceViewer::resizeEvent(QResizeEvent* event)
 void KsTraceViewer::keyReleaseEvent(QKeyEvent *event)
 {
 	if (event->key() == Qt::Key_Up || event->key() == Qt::Key_Down) {
-		QItemSelectionModel *sm = _view.selectionModel();
-		if (sm->hasSelection()) {
-			/* Only one row at the time can be selected. */
-			int row = sm->selectedRows()[0].row();
+		int row = _getSelectedDataRow();
+		if (row >= 0)
 			emit select(row); // Send a signal to the Graph widget.
-		}
 
 		return;
 	}
@@ -564,7 +583,7 @@ size_t KsTraceViewer::_searchItems(int column,
 				   const QString &searchText,
 				   condition_func cond)
 {
-	int count;
+	int count, dataRow;
 
 	_searchProgBar.show();
 	_pbAction->setVisible(true);
@@ -588,28 +607,10 @@ size_t KsTraceViewer::_searchItems(int column,
 	if (count == 0) // No items have been found. Do nothing.
 		return 0;
 
-	QItemSelectionModel *sm = _view.selectionModel();
-	if (sm->hasSelection()) {
-		/* Only one row at the time can be selected. */
-		int row = sm->selectedRows()[0].row();
-
+	dataRow = _getSelectedDataRow();
+	if (dataRow >= 0) {
 		_view.clearSelection();
-		_it = _matchList.begin();
-		/*
-		 * Move the iterator to the first element of the match list
-		 * after the selected one.
-		 */
-		while (*_it <= row) {
-			++_it;  // Move the iterator.
-			if (_it == _matchList.end()) {
-				/*
-				 * This is the last item of the list. Go back
-				 * to the beginning.
-				 */
-				_it = _matchList.begin();
-				break;
-			}
-		}
+		_setSearchIterator(dataRow);
 	} else {
 		/* Move the iterator to the beginning of the match list. */
 		_view.clearSelection();
@@ -621,6 +622,29 @@ size_t KsTraceViewer::_searchItems(int column,
 	return count;
 }
 
+void KsTraceViewer::_setSearchIterator(int row)
+{
+	if (_matchList.isEmpty())
+		return;
+
+	/*
+	 * Move the iterator to the first element of the match list
+	 * after the selected one.
+	 */
+	_it = _matchList.begin();
+	while (*_it < row) {
+		++_it;  // Move the iterator.
+		if (_it == _matchList.end()) {
+			/*
+			 * This is the last item of the list. Go back
+			 * to the beginning.
+			 */
+			_it = _matchList.begin();
+			break;
+		}
+	}
+}
+
 void KsTraceViewer::_searchItemsMapReduce(int column,
 					  const QString &searchText,
 					  condition_func cond)
@@ -668,3 +692,17 @@ void KsTraceViewer::_searchItemsMapReduce(int column,
 	for (auto &m: maps)
 		lamSearchReduce(_matchList, m.get());
 }
+
+int KsTraceViewer::_getSelectedDataRow()
+{
+	QItemSelectionModel *sm = _view.selectionModel();
+	int dataRow = -1;
+
+	if (sm->hasSelection()) {
+		/* Only one row at the time can be selected. */
+		QModelIndex  i = sm->selectedRows()[0];
+		dataRow = _proxyModel.mapRowFromSource(i.row());
+	}
+
+	return dataRow;
+}
diff --git a/kernel-shark-qt/src/KsTraceViewer.hpp b/kernel-shark-qt/src/KsTraceViewer.hpp
index 19371de..50c9115 100644
--- a/kernel-shark-qt/src/KsTraceViewer.hpp
+++ b/kernel-shark-qt/src/KsTraceViewer.hpp
@@ -140,6 +140,10 @@ private:
 
 	void _onCustomContextMenu(const QPoint &);
 
+	void _setSearchIterator(int row);
+
+	int _getSelectedDataRow();
+
 private slots:
 
 	void _searchEdit(int);
-- 
2.17.1

[PATCH 07/11] kernel-shark-qt: Optimize the search in a case of a small data-set

[Yordan Karadzhov]

Parallelizing the search (map-reduce) and showing the progress make
sense only in the case of very big data-sets. If the data-set is
small we do not want to have the overhead added by the update of
the progress bar. This overhead turns to be not so small (~1s. on
my laptop).

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsTraceViewer.cpp | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index 1f96234..64c9fb7 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -579,6 +579,12 @@ void KsTraceViewer::_resizeToContents()
 	_view.setColumnWidth(0, columnSize);
 }
 
+//! @cond Doxygen_Suppress
+
+#define KS_SEARCH_SHOW_PROGRESS_MIN 100000
+
+//! @endcond
+
 size_t KsTraceViewer::_searchItems(int column,
 				   const QString &searchText,
 				   condition_func cond)
@@ -588,7 +594,14 @@ size_t KsTraceViewer::_searchItems(int column,
 	_searchProgBar.show();
 	_pbAction->setVisible(true);
 
-	if (column == KsViewModel::TRACE_VIEW_COL_INFO ||
+	if (_proxyModel.rowCount({}) < KS_SEARCH_SHOW_PROGRESS_MIN) {
+		/*
+		 * This is a small data-set. Do a single-threaded search
+		 * without showing the progress.
+		 */
+		_proxyModel.search(column, searchText, cond, &_matchList,
+				   nullptr, nullptr);
+	} else if (column == KsViewModel::TRACE_VIEW_COL_INFO ||
 	    column == KsViewModel::TRACE_VIEW_COL_LAT) {
 		_searchStopAction->setVisible(true);
 		_proxyModel.search(column, searchText, cond, &_matchList,
-- 
2.17.1

[PATCH 08/11] kernel-shark qt: No error when Record authentication dialog is closed

[Yordan Karadzhov]

No error message must be shown in the case when the user dismissed
the authentication dialog (clicked Cancel).

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsMainWindow.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel-shark-qt/src/KsMainWindow.cpp b/kernel-shark-qt/src/KsMainWindow.cpp
index ffb10d4..d5b22dc 100644
--- a/kernel-shark-qt/src/KsMainWindow.cpp
+++ b/kernel-shark-qt/src/KsMainWindow.cpp
@@ -965,6 +965,14 @@ void KsMainWindow::_captureFinished(int exit, QProcess::ExitStatus st)
 
 	_captureLocalServer.close();
 
+	if (exit == 126) {
+		/*
+		 * Authorization could not be obtained because the user
+		 * dismissed the authentication dialog.
+		 */
+		return;
+	}
+
 	if (exit != 0 || st != QProcess::NormalExit) {
 		QString message = "Capture process failed:<br>";
 
-- 
2.17.1

[PATCH 09/11] kernel-shark-qt: Remove all system=ftrace events from Record dialog

[Yordan Karadzhov]

"ftrace" system events should not be passed as a command line option
to trace-cmd. This patch remove these events from the checkbox tree of
the Record dialog.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark-qt/src/KsCaptureDialog.cpp |  1 +
 kernel-shark-qt/src/KsWidgetsLib.cpp    | 10 ++++++++++
 kernel-shark-qt/src/KsWidgetsLib.hpp    |  2 ++
 3 files changed, 13 insertions(+)

diff --git a/kernel-shark-qt/src/KsCaptureDialog.cpp b/kernel-shark-qt/src/KsCaptureDialog.cpp
index ee1abc3..5d5ca2f 100644
--- a/kernel-shark-qt/src/KsCaptureDialog.cpp
+++ b/kernel-shark-qt/src/KsCaptureDialog.cpp
@@ -81,6 +81,7 @@ KsCaptureControl::KsCaptureControl(QWidget *parent)
 
 	_eventsWidget.setDefault(false);
 	_eventsWidget.setMinimumHeight(25 * FONT_HEIGHT);
+	_eventsWidget.removeSystem("ftrace");
 	_topLayout.addWidget(&_eventsWidget);
 
 	_pluginsLabel.adjustSize();
diff --git a/kernel-shark-qt/src/KsWidgetsLib.cpp b/kernel-shark-qt/src/KsWidgetsLib.cpp
index dd6ab0f..191ea7d 100644
--- a/kernel-shark-qt/src/KsWidgetsLib.cpp
+++ b/kernel-shark-qt/src/KsWidgetsLib.cpp
@@ -706,6 +706,16 @@ KsEventsCheckBoxWidget::KsEventsCheckBoxWidget(struct tep_handle *tep,
 	_adjustSize();
 }
 
+/** Remove a System from the Checkbox tree. */
+void KsEventsCheckBoxWidget::removeSystem(QString name) {
+	QTreeWidgetItem *item =
+		_tree.findItems(name, Qt::MatchFixedString, 0)[0];
+
+	int index = _tree.indexOfTopLevelItem(item);
+	if (index >= 0)
+		_tree.takeTopLevelItem(index);
+}
+
 /**
  * @brief Create KsTasksCheckBoxWidget.
  *
diff --git a/kernel-shark-qt/src/KsWidgetsLib.hpp b/kernel-shark-qt/src/KsWidgetsLib.hpp
index 89c196a..c09bcd5 100644
--- a/kernel-shark-qt/src/KsWidgetsLib.hpp
+++ b/kernel-shark-qt/src/KsWidgetsLib.hpp
@@ -332,6 +332,8 @@ struct KsEventsCheckBoxWidget : public KsCheckBoxTreeWidget
 
 	KsEventsCheckBoxWidget(struct tep_handle *pe,
 			       QWidget *parent = nullptr);
+
+	void removeSystem(QString name);
 };
 
 /**
-- 
2.17.1

Re: [PATCH 01/11] kernel-shark-qt: Protect all calls of tep_read_number_field()

[Steven Rostedt]

On Wed, 21 Nov 2018 15:14:19 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> tep_read_number_field() is being used to retrieve the value of a data
> field and this value has being used without checking if the function
> succeeded. This is a potential bug because tep_read_number_field() may
> fail and in such a case the retrieved field value will be arbitrary.
> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark-qt/src/plugins/sched_events.c | 52 +++++++++++++---------
>  1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel-shark-qt/src/plugins/sched_events.c b/kernel-shark-qt/src/plugins/sched_events.c
> index 1851569..c22e198 100644
> --- a/kernel-shark-qt/src/plugins/sched_events.c
> +++ b/kernel-shark-qt/src/plugins/sched_events.c
> @@ -97,10 +97,12 @@ int plugin_get_next_pid(struct tep_record *record)
>  	struct plugin_sched_context *plugin_ctx =
>  		plugin_sched_context_handler;
>  	unsigned long long val;
> +	int ret;
>  
> -	tep_read_number_field(plugin_ctx->sched_switch_next_field,
> -			      record->data, &val);
> -	return val;
> +	ret = tep_read_number_field(plugin_ctx->sched_switch_next_field,
> +				    record->data, &val);
> +
> +	return (ret == 0) ? val : ret;

BTW, here's a little optimization trick:

	return ret ? : val;

We should change the rest to do that.

-- Steve

>  }
>  

Re: [PATCH 03/11] kernel-shark-qt: Avoid race condition in kshark_get_event_name_easy()

[Steven Rostedt]

On Wed, 21 Nov 2018 15:14:21 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> Calling tep_data_event_from_type() is not thread-safe (potential
> race condition). This patch protects the access to it by using the
> mutex associated with the input file.

Tzvetomir sent patches to handle the race condition here. I'll work to
apply his.

-- Steve

> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark-qt/src/libkshark.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> index fa85171..89ae769 100644
> --- a/kernel-shark-qt/src/libkshark.c
> +++ b/kernel-shark-qt/src/libkshark.c
> @@ -1147,7 +1147,18 @@ const char *kshark_get_event_name_easy(struct kshark_entry *entry)
>  		}
>  	}
>  
> +	/*
> +	 * Calling tep_data_event_from_type() is not thread-safe (potential
> +	 * race condition).
> +	 * TODO: See if we can add a thread-safe version of the
> +	 * function. For the time being use a mutex to protect the access.
> +	 */
> +	pthread_mutex_lock(&kshark_ctx->input_mutex);
> +
>  	event = tep_data_event_from_type(kshark_ctx->pevent, event_id);
> +
> +	pthread_mutex_unlock(&kshark_ctx->input_mutex);
> +
>  	if (event)
>  		return event->name;
>  

Re: [PATCH 08/11] kernel-shark qt: No error when Record authentication dialog is closed

[Steven Rostedt]

On Wed, 21 Nov 2018 15:14:26 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> No error message must be shown in the case when the user dismissed
> the authentication dialog (clicked Cancel).
> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark-qt/src/KsMainWindow.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel-shark-qt/src/KsMainWindow.cpp b/kernel-shark-qt/src/KsMainWindow.cpp
> index ffb10d4..d5b22dc 100644
> --- a/kernel-shark-qt/src/KsMainWindow.cpp
> +++ b/kernel-shark-qt/src/KsMainWindow.cpp
> @@ -965,6 +965,14 @@ void KsMainWindow::_captureFinished(int exit, QProcess::ExitStatus st)
>  
>  	_captureLocalServer.close();
>  
> +	if (exit == 126) {

Can we make a define for than number?

Thanks,

-- Steve

> +		/*
> +		 * Authorization could not be obtained because the user
> +		 * dismissed the authentication dialog.
> +		 */
> +		return;
> +	}
> +
>  	if (exit != 0 || st != QProcess::NormalExit) {
>  		QString message = "Capture process failed:<br>";
>  

Re: [PATCH 00/11] Small modifications and bug fixes toward KS 1.0

[Steven Rostedt]

On Wed, 21 Nov 2018 15:14:17 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> This series of patches contains various independent modifications and
> bug fixes needed before releasing KernelShark 1.0.
> 
> Yordan Karadzhov (11):
>   kernel-shark-qt: Protect all calls of tep_read_number_field()
>   kernel-shark-qt: Fix the returned error value of
>     kshark_get_event_id_easy()
>   kernel-shark-qt: Avoid race condition in kshark_get_event_name_easy()
>   kernel-shark-qt: Optimize the search in the text data
>   kernel-shark-qt: Add iterator index to the search panel
>   kernel-shark-qt: Update search iterator when marker is changed
>   kernel-shark-qt: Optimize the search in a case of a small data-set
>   kernel-shark qt: No error when Record authentication dialog is closed
>   kernel-shark-qt: Remove all system=ftrace events from Record dialog

I applied patches 2, 4, 5, 6, 7, and 9 which are:

 kernel-shark-qt: Fix the returned error value of kshark_get_event_id_easy()
 kernel-shark-qt: Optimize the search in the text data
 kernel-shark-qt: Add iterator index to the search panel
 kernel-shark-qt: Update search iterator when marker is changed
 kernel-shark-qt: Optimize the search in a case of a small data-set
 kernel-shark-qt: Remove all system=ftrace events from Record dialog

The rest I commented on.

>   kernel-shark-qt: Updata Event filter mask when applaing filters to
>     Graph
>   kernel-shark-qt: Reprocess all CPU collections when the filtering
>     changes

The above two I never received ?

-- Steve

> 
>  kernel-shark-qt/src/KsCaptureDialog.cpp    |   1 +
>  kernel-shark-qt/src/KsMainWindow.cpp       |   8 ++
>  kernel-shark-qt/src/KsModels.cpp           |  28 ++--
>  kernel-shark-qt/src/KsModels.hpp           |   4 +-
>  kernel-shark-qt/src/KsTraceViewer.cpp      | 152 +++++++++++++++------
>  kernel-shark-qt/src/KsTraceViewer.hpp      |   9 +-
>  kernel-shark-qt/src/KsUtils.cpp            |   6 +
>  kernel-shark-qt/src/KsWidgetsLib.cpp       |  10 ++
>  kernel-shark-qt/src/KsWidgetsLib.hpp       |   2 +
>  kernel-shark-qt/src/libkshark.c            |  15 +-
>  kernel-shark-qt/src/plugins/sched_events.c |  52 ++++---
>  11 files changed, 204 insertions(+), 83 deletions(-)
>