[PATCH v2 0/8] Various modifications and fixes toward KS 1.0

[PATCH v2 0/8] Various modifications and fixes toward KS 1.0

[Yordan Karadzhov]

The patch-set contains reimplementation of patches that have been sent
already, but have been found to have various problems. The last two
patch are fixes of bugs that have been found recently.

Yordan Karadzhov (8):
  kernel-shark: Configuration information in ${HOME}/.cache/kernelshark
  kernel-shark: Remove the definition of KS_CONF_DIR
  kernel-shark: Add logic for the initial path of Open-File dialogs
  kernel-shark: Add logic for the plugins search path
  kernel-shark: Rename KS_DIR to KS_SOURCE_DIR
  kernel-shark: Load Last Session from command line
  kernel-shark: Use proper searching condition when the dataset is small
  kernel-shark: Handle the case when the marker points to a filtered
    entry

 kernel-shark/CMakeLists.txt             | 37 +++++-------
 kernel-shark/build/deff.h.cmake         |  6 +-
 kernel-shark/src/CMakeLists.txt         | 10 ++--
 kernel-shark/src/KsCaptureDialog.cpp    |  6 +-
 kernel-shark/src/KsMainWindow.cpp       | 80 ++++++++++++++++++++-----
 kernel-shark/src/KsMainWindow.hpp       |  4 ++
 kernel-shark/src/KsTraceViewer.cpp      | 32 ++++++----
 kernel-shark/src/KsTraceViewer.hpp      |  2 +
 kernel-shark/src/KsUtils.cpp            | 35 +++++++----
 kernel-shark/src/KsUtils.hpp            | 16 +++++
 kernel-shark/src/kernelshark.cpp        |  9 ++-
 kernel-shark/src/plugins/CMakeLists.txt |  2 +-
 12 files changed, 168 insertions(+), 71 deletions(-)

-- 
2.19.1

[PATCH v2 1/8] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov]

By default the "Last session" configuration file will be saved in
${HOME}/.cache/kernelshark. If ${HOME}/.cache/kernelshark doesn't exist,
it will be created automatically. The user can select another directory
to be used to store the cached data. This can be done by setting the
environment variable KS_USER_CACHE_DIR. In this case if the path (the
value of KS_USER_CACHE_DIR) doesn't exist the user will be asked before
create it.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsMainWindow.cpp | 68 +++++++++++++++++++++++++++----
 kernel-shark/src/KsMainWindow.hpp |  4 ++
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 7afb721..cf2db74 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -133,13 +133,15 @@ KsMainWindow::KsMainWindow(QWidget *parent)
 KsMainWindow::~KsMainWindow()
 {
 	kshark_context *kshark_ctx(nullptr);
-	QString file = KS_CONF_DIR;
+	QString file = lastSessionFile();
 
-	file += "/lastsession.json";
+	if (!file.isEmpty()) {
+		QByteArray fileBA = file.toLocal8Bit();
 
-	_updateSession();
-	kshark_save_config_file(file.toLocal8Bit().data(),
-				_session.getConfDocPtr());
+		_updateSession();
+		kshark_save_config_file(fileBA.data(),
+					_session.getConfDocPtr());
+	}
 
 	_data.clear();
 
@@ -368,12 +370,60 @@ void KsMainWindow::_open()
 		loadDataFile(fileName);
 }
 
-void KsMainWindow::_restoreSession()
+QString KsMainWindow::_getCacheDir()
+{
+	QString dir;
+
+	auto lamMakePath = [&] (bool ask) {
+		if (ask) {
+			QMessageBox::StandardButton reply;
+			QString err("KernelShark cache directory not found!\n");
+			QString question =
+				QString("Do you want to create %1").arg(dir);
+
+			reply = QMessageBox::question(this, "KernelShark",
+						      err + question,
+						      QMessageBox::Yes |
+						      QMessageBox::No);
+
+			if (reply == QMessageBox::No) {
+				dir.clear();
+				return;
+			}
+		}
+
+		QDir().mkpath(dir);
+	};
+
+	dir = getenv("KS_USER_CACHE_DIR");
+	if (!dir.isEmpty()) {
+		if (!QDir(dir).exists())
+			lamMakePath(true);
+	} else {
+		dir = QString(getpwuid(getuid())->pw_dir) +
+		      "/.cache/kernelshark";
+		if (!QDir(dir).exists())
+			lamMakePath(false);
+	}
+
+	return dir;
+}
+
+/** Get the description file of the last session. */
+QString KsMainWindow::lastSessionFile()
 {
-	QString file = KS_CONF_DIR;
-	file += "/lastsession.json";
+	QString file;
 
-	loadSession(file);
+	file = _getCacheDir();
+	if (!file.isEmpty())
+		file += "/lastsession.json";
+
+	return file;
+}
+
+void KsMainWindow::_restoreSession()
+{
+	loadSession(lastSessionFile());
 	_graph.updateGeom();
 }
 
diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
index 78cd442..ec6506e 100644
--- a/kernel-shark/src/KsMainWindow.hpp
+++ b/kernel-shark/src/KsMainWindow.hpp
@@ -37,6 +37,8 @@ public:
 
 	void loadSession(const QString &fileName);
 
+	QString lastSessionFile();
+
 	/**
 	 * @brief
 	 *
@@ -230,6 +232,8 @@ private:
 
 	void _filterSyncCBoxUpdate(kshark_context *kshark_ctx);
 
+	QString _getCacheDir();
+
 private slots:
 	void _captureFinished(int, QProcess::ExitStatus);
 };
-- 
2.19.1

Re: [PATCH v2 1/8] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Thu, Apr 18, 2019 at 6:22 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> By default the "Last session" configuration file will be saved in
> ${HOME}/.cache/kernelshark. If ${HOME}/.cache/kernelshark doesn't exist,
> it will be created automatically. The user can select another directory
> to be used to store the cached data. This can be done by setting the
> environment variable KS_USER_CACHE_DIR. In this case if the path (the
> value of KS_USER_CACHE_DIR) doesn't exist the user will be asked before
> create it.
>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>

Nit: two identical Signed-off-by tags

> ---
>  kernel-shark/src/KsMainWindow.cpp | 68 +++++++++++++++++++++++++++----
>  kernel-shark/src/KsMainWindow.hpp |  4 ++
>  2 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 7afb721..cf2db74 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp

[...]

> -void KsMainWindow::_restoreSession()
> +QString KsMainWindow::_getCacheDir()
> +{
> +       QString dir;
> +
> +       auto lamMakePath = [&] (bool ask) {
> +               if (ask) {
> +                       QMessageBox::StandardButton reply;
> +                       QString err("KernelShark cache directory not found!\n");
> +                       QString question =
> +                               QString("Do you want to create %1").arg(dir);
> +
> +                       reply = QMessageBox::question(this, "KernelShark",
> +                                                     err + question,
> +                                                     QMessageBox::Yes |
> +                                                     QMessageBox::No);
> +
> +                       if (reply == QMessageBox::No) {
> +                               dir.clear();
> +                               return;
> +                       }
> +               }
> +
> +               QDir().mkpath(dir);
> +       };
> +
> +       dir = getenv("KS_USER_CACHE_DIR");
> +       if (!dir.isEmpty()) {
> +               if (!QDir(dir).exists())
> +                       lamMakePath(true);
> +       } else {
> +               dir = QString(getpwuid(getuid())->pw_dir) +
> +                     "/.cache/kernelshark";

Nit: s/QString(getpwuid(getuid())->pw_dir)/QDir::homePath()/

Everything else looks great with the patch
Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

--Slavi

Re: [PATCH v2 1/8] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Fri, 19 Apr 2019 10:19:38 +0300
Slavomir Kaslev <slavomir.kaslev@gmail.com> wrote:

> Nit: s/QString(getpwuid(getuid())->pw_dir)/QDir::homePath()/
> 
> Everything else looks great with the patch
> Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Yordan,

If someone gives you a review by, please add it to the next revision, if
you didn't change the patch. And if you do change it, ask for a new
review-by.

Thanks!

-- Steve

[PATCH v2 2/8] kernel-shark: Remove the definition of KS_CONF_DIR

[Yordan Karadzhov]

KS_CONF_DIR is no longer used, so we do not need to define it in the
Cmake-generated header file.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/CMakeLists.txt     | 5 -----
 kernel-shark/build/deff.h.cmake | 3 ---
 2 files changed, 8 deletions(-)

diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 10cb696..1aee858 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -12,11 +12,6 @@ message("\n project: Kernel Shark: (version: ${KS_VERSION_STRING})\n")
 
 set(KS_DIR ${CMAKE_SOURCE_DIR})
 
-# Make a directory to hold configuration files. To change this do:
-# cmake .. -DKS_CONF_DIR=/your/preferred/path
-set(KS_CONF_DIR "${KS_DIR}/.ksconf" CACHE STRING "Directory for configuration files.")
-file(MAKE_DIRECTORY ${KS_CONF_DIR})
-
 include(${KS_DIR}/build/FindTraceCmd.cmake)
 include(${KS_DIR}/build/FindJSONC.cmake)
 
diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
index 80d624c..8041cfc 100644
--- a/kernel-shark/build/deff.h.cmake
+++ b/kernel-shark/build/deff.h.cmake
@@ -14,9 +14,6 @@
 /** KernelShark source code path. */
 #cmakedefine KS_DIR "@KS_DIR@"
 
-/** KernelShark configuration directory path. */
-#cmakedefine KS_CONF_DIR "@KS_CONF_DIR@"
-
 /** Location of the trace-cmd executable. */
 #cmakedefine TRACECMD_BIN_DIR "@TRACECMD_BIN_DIR@"
 
-- 
2.19.1

[PATCH v2 3/8] kernel-shark: Add logic for the initial path of Open-File dialogs

[Yordan Karadzhov]

If the application has been started from the source code directory,
all OpenFile dialogs will start there. If the application has been
started from its installation location, all Open File dialogs will
start at ${HOME}.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsCaptureDialog.cpp |  6 +++---
 kernel-shark/src/KsMainWindow.cpp    | 12 ++++++------
 kernel-shark/src/KsUtils.hpp         | 15 +++++++++++++++
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index 1272c2e..57bf7c7 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -208,7 +208,7 @@ void KsCaptureControl::_importSettings()
 	/* Get the configuration document. */
 	fileName = QFileDialog::getOpenFileName(this,
 						"Import from Filter",
-						KS_DIR,
+						KsUtils::dialogDir(),
 						"Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -259,7 +259,7 @@ void KsCaptureControl::_exportSettings()
 	QString fileName =
 		QFileDialog::getSaveFileName(this,
 					     "Export to File",
-					     KS_DIR,
+					     KsUtils::dialogDir(),
 					     "Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -314,7 +314,7 @@ void KsCaptureControl::_browse()
 	QString fileName =
 		QFileDialog::getSaveFileName(this,
 					     "Save File",
-					     KS_DIR,
+					     KsUtils::dialogDir(),
 					     "trace-cmd files (*.dat);;All files (*)");
 
 	if (!fileName.isEmpty())
diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index cf2db74..80bd019 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -363,7 +363,7 @@ void KsMainWindow::_open()
 	QString fileName =
 		QFileDialog::getOpenFileName(this,
 					     "Open File",
-					     KS_DIR,
+					     KsUtils::dialogDir(),
 					     "trace-cmd files (*.dat);;All files (*)");
 
 	if (!fileName.isEmpty())
@@ -432,7 +432,7 @@ void KsMainWindow::_importSession()
 	QString fileName =
 		QFileDialog::getOpenFileName(this,
 					     "Import Session",
-					     KS_DIR,
+					     KsUtils::dialogDir(),
 					     "Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -463,7 +463,7 @@ void KsMainWindow::_exportSession()
 	QString fileName =
 		QFileDialog::getSaveFileName(this,
 					     "Export Filter",
-					     KS_DIR,
+					     KsUtils::dialogDir(),
 					     "Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -512,7 +512,7 @@ void KsMainWindow::_importFilter()
 	if (!kshark_instance(&kshark_ctx))
 		return;
 
-	fileName = QFileDialog::getOpenFileName(this, "Import Filter", KS_DIR,
+	fileName = QFileDialog::getOpenFileName(this, "Import Filter", KsUtils::dialogDir(),
 						"Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -540,7 +540,7 @@ void KsMainWindow::_exportFilter()
 	if (!kshark_instance(&kshark_ctx))
 		return;
 
-	fileName = QFileDialog::getSaveFileName(this, "Export Filter", KS_DIR,
+	fileName = QFileDialog::getSaveFileName(this, "Export Filter", KsUtils::dialogDir(),
 						"Kernel Shark Config files (*.json);;");
 
 	if (fileName.isEmpty())
@@ -861,7 +861,7 @@ void KsMainWindow::_pluginAdd()
 
 	fileNames =
 		QFileDialog::getOpenFileNames(this, "Add KernelShark plugins",
-					      KS_DIR,
+					      KsUtils::dialogDir(),
 					      "KernelShark Plugins (*.so);;");
 
 	if (fileNames.isEmpty())
diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
index c8b5e88..877c62a 100644
--- a/kernel-shark/src/KsUtils.hpp
+++ b/kernel-shark/src/KsUtils.hpp
@@ -111,6 +111,21 @@ inline QString Ts2String(int64_t ts, int prec)
 
 bool matchCPUVisible(struct kshark_context *kshark_ctx,
 			      struct kshark_entry *e, int cpu);
+
+/**
+ * @brief Get the directory to be used when opening QFileDialog. If the
+ *	  application has been started from the source code directory, all
+ *	  Open File dialogs will start there. If the application has been
+ *	  started from its installation location, all Open File dialogs will
+ *	  start at ${HOME}.
+ */
+inline QString dialogDir()
+{
+	QString path = QCoreApplication::applicationFilePath();
+
+	return (path.contains(KS_DIR)) ? KS_DIR : QDir::homePath();
+}
+
 }; // KsUtils
 
 /** Identifier of the Dual Marker active state. */
-- 
2.19.1

Re: [PATCH v2 3/8] kernel-shark: Add logic for the initial path of Open-File dialogs

[Slavomir Kaslev]

On Thu, Apr 18, 2019 at 6:22 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> If the application has been started from the source code directory,
> all OpenFile dialogs will start there. If the application has been
> started from its installation location, all Open File dialogs will
> start at ${HOME}.
>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/KsCaptureDialog.cpp |  6 +++---
>  kernel-shark/src/KsMainWindow.cpp    | 12 ++++++------
>  kernel-shark/src/KsUtils.hpp         | 15 +++++++++++++++
>  3 files changed, 24 insertions(+), 9 deletions(-)

[...]

> diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
> index c8b5e88..877c62a 100644
> --- a/kernel-shark/src/KsUtils.hpp
> +++ b/kernel-shark/src/KsUtils.hpp
> @@ -111,6 +111,21 @@ inline QString Ts2String(int64_t ts, int prec)
>
>  bool matchCPUVisible(struct kshark_context *kshark_ctx,
>                               struct kshark_entry *e, int cpu);
> +
> +/**
> + * @brief Get the directory to be used when opening QFileDialog. If the
> + *       application has been started from the source code directory, all
> + *       Open File dialogs will start there. If the application has been
> + *       started from its installation location, all Open File dialogs will
> + *       start at ${HOME}.
> + */
> +inline QString dialogDir()
> +{
> +       QString path = QCoreApplication::applicationFilePath();
> +
> +       return (path.contains(KS_DIR)) ? KS_DIR : QDir::homePath();

Nit: no need for the parenthesis around `path.contains(KS_DIR)`

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

As a user of KernelShark who keeps his traces in ~/tmp, I still think
that opening the last directory used and saving it between KernelShark
invocations would be a great feature. But I don't think there's any
KernelShark global config being saved atm so having this as a follow
up in the future is fine.

Cheers,

-- Slavi

Re: [PATCH v2 3/8] kernel-shark: Add logic for the initial path of Open-File dialogs

[Steven Rostedt]

On Fri, 19 Apr 2019 10:26:40 +0300
Slavomir Kaslev <slavomir.kaslev@gmail.com> wrote:

> On Thu, Apr 18, 2019 at 6:22 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> >
> > If the application has been started from the source code directory,
> > all OpenFile dialogs will start there. If the application has been
> > started from its installation location, all Open File dialogs will
> > start at ${HOME}.
> >
> > Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> > ---
> >  kernel-shark/src/KsCaptureDialog.cpp |  6 +++---
> >  kernel-shark/src/KsMainWindow.cpp    | 12 ++++++------
> >  kernel-shark/src/KsUtils.hpp         | 15 +++++++++++++++
> >  3 files changed, 24 insertions(+), 9 deletions(-)  
> 
> [...]
> 
> > diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
> > index c8b5e88..877c62a 100644
> > --- a/kernel-shark/src/KsUtils.hpp
> > +++ b/kernel-shark/src/KsUtils.hpp
> > @@ -111,6 +111,21 @@ inline QString Ts2String(int64_t ts, int prec)
> >
> >  bool matchCPUVisible(struct kshark_context *kshark_ctx,
> >                               struct kshark_entry *e, int cpu);
> > +
> > +/**
> > + * @brief Get the directory to be used when opening QFileDialog. If the
> > + *       application has been started from the source code directory, all
> > + *       Open File dialogs will start there. If the application has been
> > + *       started from its installation location, all Open File dialogs will
> > + *       start at ${HOME}.
> > + */
> > +inline QString dialogDir()
> > +{
> > +       QString path = QCoreApplication::applicationFilePath();
> > +
> > +       return (path.contains(KS_DIR)) ? KS_DIR : QDir::homePath();  
> 
> Nit: no need for the parenthesis around `path.contains(KS_DIR)`
> 
> Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>
> 
> As a user of KernelShark who keeps his traces in ~/tmp, I still think
> that opening the last directory used and saving it between KernelShark
> invocations would be a great feature. But I don't think there's any
> KernelShark global config being saved atm so having this as a follow
> up in the future is fine.
> 

Perhaps not save it between bringing up kernelshark, but at least to
remember it during the execution time.

Also, I think having the cwd be the default location would be a good
idea, then you could just run kernelshark from /tmp and it will be the
location you see.

-- Steve

[PATCH v2 4/8] kernel-shark: Add logic for the plugins search path

[Yordan Karadzhov]

If the application has been started from the source code directory, all
build-in plugins will be loaded from kernel-shark/lib/
If the application has been started from its installation location, all
build-in plugins will be loaded from INSTALL_PREFIX/lib/kshark/plugins/

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/build/deff.h.cmake         |  3 +++
 kernel-shark/src/KsUtils.cpp            | 35 +++++++++++++++++--------
 kernel-shark/src/KsUtils.hpp            |  1 +
 kernel-shark/src/plugins/CMakeLists.txt |  2 +-
 4 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
index 8041cfc..ba211f4 100644
--- a/kernel-shark/build/deff.h.cmake
+++ b/kernel-shark/build/deff.h.cmake
@@ -14,6 +14,9 @@
 /** KernelShark source code path. */
 #cmakedefine KS_DIR "@KS_DIR@"
 
+/** KernelShark installation prefix path. */
+#cmakedefine _INSTALL_PREFIX "@_INSTALL_PREFIX@"
+
 /** Location of the trace-cmd executable. */
 #cmakedefine TRACECMD_BIN_DIR "@TRACECMD_BIN_DIR@"
 
diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
index 6af0c66..b05c0dc 100644
--- a/kernel-shark/src/KsUtils.cpp
+++ b/kernel-shark/src/KsUtils.cpp
@@ -423,13 +423,12 @@ void KsPluginManager::_parsePluginList()
  */
 void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
 {
-	auto lamRegBuiltIn = [&kshark_ctx](const QString &plugin)
+	auto lamRegBuiltIn = [&kshark_ctx, this](const QString &plugin)
 	{
 		char *lib;
 		int n;
 
-		n = asprintf(&lib, "%s/lib/plugin-%s.so",
-			     KS_DIR, plugin.toStdString().c_str());
+		lib = _pluginLibFromName(plugin, n);
 		if (n <= 0)
 			return;
 
@@ -458,13 +457,12 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
  */
 void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
 {
-	auto lamUregBuiltIn = [&kshark_ctx](const QString &plugin)
+	auto lamUregBuiltIn = [&kshark_ctx, this](const QString &plugin)
 	{
 		char *lib;
 		int n;
 
-		n = asprintf(&lib, "%s/lib/plugin-%s.so",
-			     KS_DIR, plugin.toStdString().c_str());
+		lib = _pluginLibFromName(plugin, n);
 		if (n <= 0)
 			return;
 
@@ -487,6 +485,23 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
 			lamUregUser);
 }
 
+char *KsPluginManager::_pluginLibFromName(const QString &plugin, int &n)
+{
+	QString path = QCoreApplication::applicationFilePath();
+	std::string pluginStr = plugin.toStdString();
+	char *lib;
+
+	if (path.contains(KS_DIR)) {
+		n = asprintf(&lib, "%s/lib/plugin-%s.so",
+			     KS_DIR, pluginStr.c_str());
+	} else {
+		n = asprintf(&lib, "%s/lib/kshark/plugins/plugin-%s.so",
+			     _INSTALL_PREFIX, pluginStr.c_str());
+	}
+
+	return lib;
+}
+
 /**
  * @brief Register a Plugin.
  *
@@ -508,8 +523,7 @@ void KsPluginManager::registerPlugin(const QString &plugin)
 			 * The argument is the name of the plugin. From the
 			 * name get the library .so file.
 			 */
-			n = asprintf(&lib, "%s/lib/plugin-%s.so",
-					KS_DIR, plugin.toStdString().c_str());
+			lib = _pluginLibFromName(plugin, n);
 			if (n > 0) {
 				kshark_register_plugin(kshark_ctx, lib);
 				_registeredKsPlugins[i] = true;
@@ -570,8 +584,7 @@ void KsPluginManager::unregisterPlugin(const QString &plugin)
 			 * The argument is the name of the plugin. From the
 			 * name get the library .so file.
 			 */
-			n = asprintf(&lib, "%s/lib/plugin-%s.so", KS_DIR,
-				     plugin.toStdString().c_str());
+			lib = _pluginLibFromName(plugin, n);
 			if (n > 0) {
 				kshark_unregister_plugin(kshark_ctx, lib);
 				_registeredKsPlugins[i] = false;
@@ -579,7 +592,7 @@ void KsPluginManager::unregisterPlugin(const QString &plugin)
 			}
 
 			return;
-		} else if  (plugin.contains("/lib/plugin-" +
+		} else if (plugin.contains("/lib/plugin-" +
 			                   _ksPluginList[i], Qt::CaseInsensitive)) {
 			/*
 			 * The argument is the name of the library .so file.
diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
index 877c62a..85da9ae 100644
--- a/kernel-shark/src/KsUtils.hpp
+++ b/kernel-shark/src/KsUtils.hpp
@@ -238,6 +238,7 @@ signals:
 
 private:
 	void _parsePluginList();
+	char *_pluginLibFromName(const QString &plugin, int &n);
 
 	template <class T>
 	void _forEachInList(const QStringList &pl,
diff --git a/kernel-shark/src/plugins/CMakeLists.txt b/kernel-shark/src/plugins/CMakeLists.txt
index 6098275..64cf98d 100644
--- a/kernel-shark/src/plugins/CMakeLists.txt
+++ b/kernel-shark/src/plugins/CMakeLists.txt
@@ -29,6 +29,6 @@ BUILD_PLUGIN(NAME missed_events
 list(APPEND PLUGIN_LIST "missed_events default") # This plugin will be loaded by default
 
 install(TARGETS sched_events missed_events
-        LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/kshark/)
+        LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/kshark/plugins/)
 
 set(PLUGINS ${PLUGIN_LIST} PARENT_SCOPE)
-- 
2.19.1

Re: [PATCH v2 4/8] kernel-shark: Add logic for the plugins search path

[Slavomir Kaslev]

On Thu, Apr 18, 2019 at 6:22 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> If the application has been started from the source code directory, all
> build-in plugins will be loaded from kernel-shark/lib/
> If the application has been started from its installation location, all
> build-in plugins will be loaded from INSTALL_PREFIX/lib/kshark/plugins/
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/build/deff.h.cmake         |  3 +++
>  kernel-shark/src/KsUtils.cpp            | 35 +++++++++++++++++--------
>  kernel-shark/src/KsUtils.hpp            |  1 +
>  kernel-shark/src/plugins/CMakeLists.txt |  2 +-
>  4 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
> index 8041cfc..ba211f4 100644
> --- a/kernel-shark/build/deff.h.cmake
> +++ b/kernel-shark/build/deff.h.cmake
> @@ -14,6 +14,9 @@
>  /** KernelShark source code path. */
>  #cmakedefine KS_DIR "@KS_DIR@"
>
> +/** KernelShark installation prefix path. */
> +#cmakedefine _INSTALL_PREFIX "@_INSTALL_PREFIX@"
> +
>  /** Location of the trace-cmd executable. */
>  #cmakedefine TRACECMD_BIN_DIR "@TRACECMD_BIN_DIR@"
>
> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
> index 6af0c66..b05c0dc 100644
> --- a/kernel-shark/src/KsUtils.cpp
> +++ b/kernel-shark/src/KsUtils.cpp
> @@ -423,13 +423,12 @@ void KsPluginManager::_parsePluginList()
>   */
>  void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>  {
> -       auto lamRegBuiltIn = [&kshark_ctx](const QString &plugin)
> +       auto lamRegBuiltIn = [&kshark_ctx, this](const QString &plugin)

Prefixing lambda function names with 'lam' is an interesting modern
form of Hungarian notation[1]. It's not useful for the reader or the
compiler and it doesn't make a great style. No need to change this
patch but having a clean up patch that does a bulk rename would be
nice.

Other than that the patch looks good to me and also for patches 2, 4, 5 and 6

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

-- Slavi

[1]https://en.wikipedia.org/wiki/Hungarian_notation

Re: [PATCH v2 4/8] kernel-shark: Add logic for the plugins search path

[Steven Rostedt]

On Fri, 19 Apr 2019 10:43:17 +0300
Slavomir Kaslev <slavomir.kaslev@gmail.com> wrote:

> > diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
> > index 6af0c66..b05c0dc 100644
> > --- a/kernel-shark/src/KsUtils.cpp
> > +++ b/kernel-shark/src/KsUtils.cpp
> > @@ -423,13 +423,12 @@ void KsPluginManager::_parsePluginList()
> >   */
> >  void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
> >  {
> > -       auto lamRegBuiltIn = [&kshark_ctx](const QString &plugin)
> > +       auto lamRegBuiltIn = [&kshark_ctx, this](const QString &plugin)  
> 
> Prefixing lambda function names with 'lam' is an interesting modern
> form of Hungarian notation[1]. It's not useful for the reader or the
> compiler and it doesn't make a great style. No need to change this
> patch but having a clean up patch that does a bulk rename would be
> nice.

I don't know. Having them labeled does make it easy to know that they
are lambda functions when used later.

-- Steve

[PATCH v2 5/8] kernel-shark: Rename KS_DIR to KS_SOURCE_DIR

[Yordan Karadzhov]

KS_SOURCE_DIR is a more appropriate name since it defines the path to
the KernelShark source code used to build the application.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/CMakeLists.txt     | 32 ++++++++++++++++----------------
 kernel-shark/build/deff.h.cmake |  2 +-
 kernel-shark/src/CMakeLists.txt | 10 +++++-----
 kernel-shark/src/KsUtils.cpp    |  4 ++--
 kernel-shark/src/KsUtils.hpp    |  2 +-
 5 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 1aee858..825cabd 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -10,10 +10,10 @@ set(KS_VERSION_PATCH 8)
 set(KS_VERSION_STRING ${KS_VERSION_MAJOR}.${KS_VERSION_MINOR}.${KS_VERSION_PATCH})
 message("\n project: Kernel Shark: (version: ${KS_VERSION_STRING})\n")
 
-set(KS_DIR ${CMAKE_SOURCE_DIR})
+set(KS_SOURCE_DIR ${CMAKE_SOURCE_DIR})
 
-include(${KS_DIR}/build/FindTraceCmd.cmake)
-include(${KS_DIR}/build/FindJSONC.cmake)
+include(${KS_SOURCE_DIR}/build/FindTraceCmd.cmake)
+include(${KS_SOURCE_DIR}/build/FindJSONC.cmake)
 
 find_package(Doxygen)
 
@@ -29,8 +29,8 @@ if (Qt5Widgets_FOUND)
 
 endif (Qt5Widgets_FOUND)
 
-set(LIBRARY_OUTPUT_PATH    "${KS_DIR}/lib")
-set(EXECUTABLE_OUTPUT_PATH "${KS_DIR}/bin")
+set(LIBRARY_OUTPUT_PATH    "${KS_SOURCE_DIR}/lib")
+set(EXECUTABLE_OUTPUT_PATH "${KS_SOURCE_DIR}/bin")
 
 set(CMAKE_C_FLAGS   "${CMAKE_C_FLAGS} -Wall -g -pthread")
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -g -std=c++11 -pthread")
@@ -49,8 +49,8 @@ endif (NOT _DEBUG)
 SET(CMAKE_INSTALL_RPATH "${_INSTALL_PREFIX}/lib/kshark/")
 SET(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE)
 
-include_directories(${KS_DIR}/src/
-                    ${KS_DIR}/build/src/
+include_directories(${KS_SOURCE_DIR}/src/
+                    ${KS_SOURCE_DIR}/build/src/
                     ${JSONC_INCLUDE_DIR}
                     ${TRACECMD_INCLUDE_DIR}
                     ${TRACEEVENT_INCLUDE_DIR})
@@ -60,8 +60,8 @@ message(STATUS "C flags      : " ${CMAKE_C_FLAGS})
 message(STATUS "CXX flags    : " ${CMAKE_CXX_FLAGS})
 message(STATUS "Linker flags : " ${CMAKE_EXE_LINKER_FLAGS})
 
-add_subdirectory(${KS_DIR}/src)
-add_subdirectory(${KS_DIR}/examples)
+add_subdirectory(${KS_SOURCE_DIR}/src)
+add_subdirectory(${KS_SOURCE_DIR}/examples)
 
 if (_DOXYGEN_DOC AND DOXYGEN_FOUND)
 
@@ -72,16 +72,16 @@ if (_DOXYGEN_DOC AND DOXYGEN_FOUND)
                        WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/doc)
 
     set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_MAKE_CLEAN_FILES
-                                          "${KS_DIR}/doc/dox_build.log"
-                                          "${KS_DIR}/doc/html"
-                                          "${KS_DIR}/doc/latex")
+                                          "${KS_SOURCE_DIR}/doc/dox_build.log"
+                                          "${KS_SOURCE_DIR}/doc/html"
+                                          "${KS_SOURCE_DIR}/doc/latex")
 
 endif ()
 
-configure_file( ${KS_DIR}/build/ks.desktop.cmake
-                ${KS_DIR}/kernelshark.desktop)
+configure_file(${KS_SOURCE_DIR}/build/ks.desktop.cmake
+               ${KS_SOURCE_DIR}/kernelshark.desktop)
 
-configure_file( ${KS_DIR}/build/org.freedesktop.kshark-record.policy.cmake
-                ${KS_DIR}/org.freedesktop.kshark-record.policy)
+configure_file(${KS_SOURCE_DIR}/build/org.freedesktop.kshark-record.policy.cmake
+               ${KS_SOURCE_DIR}/org.freedesktop.kshark-record.policy)
 
 message("")
diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
index ba211f4..47c9d9f 100644
--- a/kernel-shark/build/deff.h.cmake
+++ b/kernel-shark/build/deff.h.cmake
@@ -12,7 +12,7 @@
 #cmakedefine KS_VERSION_STRING "@KS_VERSION_STRING@"
 
 /** KernelShark source code path. */
-#cmakedefine KS_DIR "@KS_DIR@"
+#cmakedefine KS_SOURCE_DIR "@KS_SOURCE_DIR@"
 
 /** KernelShark installation prefix path. */
 #cmakedefine _INSTALL_PREFIX "@_INSTALL_PREFIX@"
diff --git a/kernel-shark/src/CMakeLists.txt b/kernel-shark/src/CMakeLists.txt
index b7dbd7e..5f97916 100644
--- a/kernel-shark/src/CMakeLists.txt
+++ b/kernel-shark/src/CMakeLists.txt
@@ -81,13 +81,13 @@ if (Qt5Widgets_FOUND AND Qt5Network_FOUND)
             RUNTIME DESTINATION ${_INSTALL_PREFIX}/bin/
             LIBRARY DESTINATION ${_INSTALL_PREFIX}/lib/kshark/)
 
-    install(FILES "${KS_DIR}/kernelshark.desktop"
+    install(FILES "${KS_SOURCE_DIR}/kernelshark.desktop"
             DESTINATION /usr/share/applications/)
 
-    install(FILES "${KS_DIR}/org.freedesktop.kshark-record.policy"
+    install(FILES "${KS_SOURCE_DIR}/org.freedesktop.kshark-record.policy"
             DESTINATION /usr/share/polkit-1/actions/)
 
-    install(PROGRAMS "${KS_DIR}/bin/kshark-su-record"
+    install(PROGRAMS "${KS_SOURCE_DIR}/bin/kshark-su-record"
             DESTINATION ${_INSTALL_PREFIX}/bin/)
 
 endif (Qt5Widgets_FOUND AND Qt5Network_FOUND)
@@ -96,5 +96,5 @@ add_subdirectory(plugins)
 
 find_program(DO_AS_ROOT pkexec)
 
-configure_file( ${KS_DIR}/build/deff.h.cmake
-                ${KS_DIR}/src/KsCmakeDef.hpp)
+configure_file( ${KS_SOURCE_DIR}/build/deff.h.cmake
+                ${KS_SOURCE_DIR}/src/KsCmakeDef.hpp)
diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
index b05c0dc..387a1e0 100644
--- a/kernel-shark/src/KsUtils.cpp
+++ b/kernel-shark/src/KsUtils.cpp
@@ -491,9 +491,9 @@ char *KsPluginManager::_pluginLibFromName(const QString &plugin, int &n)
 	std::string pluginStr = plugin.toStdString();
 	char *lib;
 
-	if (path.contains(KS_DIR)) {
+	if (path.contains(KS_SOURCE_DIR)) {
 		n = asprintf(&lib, "%s/lib/plugin-%s.so",
-			     KS_DIR, pluginStr.c_str());
+			     KS_SOURCE_DIR, pluginStr.c_str());
 	} else {
 		n = asprintf(&lib, "%s/lib/kshark/plugins/plugin-%s.so",
 			     _INSTALL_PREFIX, pluginStr.c_str());
diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
index 85da9ae..e776d62 100644
--- a/kernel-shark/src/KsUtils.hpp
+++ b/kernel-shark/src/KsUtils.hpp
@@ -123,7 +123,7 @@ inline QString dialogDir()
 {
 	QString path = QCoreApplication::applicationFilePath();
 
-	return (path.contains(KS_DIR)) ? KS_DIR : QDir::homePath();
+	return path.contains(KS_SOURCE_DIR) ? KS_SOURCE_DIR : QDir::homePath();
 }
 
 }; // KsUtils
-- 
2.19.1

[PATCH v2 6/8] kernel-shark: Load Last Session from command line

[Yordan Karadzhov]

./kernelshark -l  will load directly the Last Session

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

diff --git a/kernel-shark/src/kernelshark.cpp b/kernel-shark/src/kernelshark.cpp
index 2ec91de..1ec6678 100644
--- a/kernel-shark/src/kernelshark.cpp
+++ b/kernel-shark/src/kernelshark.cpp
@@ -28,6 +28,7 @@ void usage(const char *prog)
 	printf("  -p	register plugin, use plugin name, absolute or relative path\n");
 	printf("  -u	unregister plugin, use plugin name or absolute path\n");
 	printf("  -s	import a session\n");
+	printf("  -l	import the last session\n");
 }
 
 int main(int argc, char **argv)
@@ -40,7 +41,7 @@ int main(int argc, char **argv)
 	int c;
 	bool fromSession = false;
 
-	while ((c = getopt(argc, argv, "hvi:p:u:s:")) != -1) {
+	while ((c = getopt(argc, argv, "hvi:p:u:s:l")) != -1) {
 		switch(c) {
 		case 'h':
 			usage(argv[0]);
@@ -65,6 +66,12 @@ int main(int argc, char **argv)
 		case 's':
 			ks.loadSession(QString(optarg));
 			fromSession = true;
+			break;
+
+		case 'l':
+			ks.loadSession(ks.lastSessionFile());
+			fromSession = true;
+			break;
 
 		default:
 			break;
-- 
2.19.1

[PATCH v2 7/8] kernel-shark: Use proper searching condition when the dataset is small

[Yordan Karadzhov]

If the data-set is small we do not want to have the overhead added by
the update of the progress bar. Because of this we bypass the state
switching of the FSM. However, in this case the the search condition
has to be updated by hand.

Reported-by: Slavomir Kaslev <kaslevs@vmware.com>
Fixes: 1615b02b (kernel-shark-qt: Optimize the search in a case of a small data-set)
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsTraceViewer.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
index 4e2c93e..04a38b8 100644
--- a/kernel-shark/src/KsTraceViewer.cpp
+++ b/kernel-shark/src/KsTraceViewer.cpp
@@ -596,8 +596,11 @@ size_t KsTraceViewer::_searchItems()
 	if (_proxyModel.rowCount({}) < KS_SEARCH_SHOW_PROGRESS_MIN) {
 		/*
 		 * This is a small data-set. Do a single-threaded search
-		 * without showing the progress.
+		 * without showing the progress. We will bypass the state
+		 * switching, hence the search condition has to be updated
+		 * by hand.
 		 */
+		_searchFSM.updateCondition();
 		_proxyModel.search(column, searchText, _searchFSM.condition(), &_matchList,
 				   nullptr, nullptr);
 	} else {
-- 
2.19.1

[PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Yordan Karadzhov]

Markers can point to entries that are filtered out. When switching
the marker it may happen that new Active Marker points to an entry
that is filtered. In this case no actions must be taken and a warning
message for the user must be displayed.

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

diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
index 04a38b8..85da64f 100644
--- a/kernel-shark/src/KsTraceViewer.cpp
+++ b/kernel-shark/src/KsTraceViewer.cpp
@@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
   _graphFollowsCheckBox(this),
   _graphFollows(true),
   _mState(nullptr),
-  _data(nullptr)
+  _data(nullptr),
+  _em(this)
 {
 	this->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Maximum);
 
@@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
 		QModelIndex index =
 			_proxyModel.mapFromSource(_model.index(row, 0));
 
-		/*
-		 * The row of the active marker will be colored according to
-		 * the assigned property of the current state of the Dual
-		 * marker. Auto-scrolling is temporarily disabled because we
-		 * do not want to scroll to the position of the marker yet.
-		 */
-		_view.setAutoScroll(false);
-		_view.selectRow(index.row());
-		_view.setAutoScroll(true);
+		if (index.isValid()) {
+			/*
+			 * The row of the active marker will be colored according to
+			 * the assigned property of the current state of the Dual
+			 * marker. Auto-scrolling is temporarily disabled because we
+			 * do not want to scroll to the position of the marker yet.
+			 */
+			_view.setAutoScroll(false);
+			_view.selectRow(index.row());
+			_view.setAutoScroll(true);
+		} else {
+			_view.clearSelection();
+			QString err("The marker's entry is filtered out.");
+			_em.showMessage(err);
+		}
 	} else {
 		_view.clearSelection();
 	}
diff --git a/kernel-shark/src/KsTraceViewer.hpp b/kernel-shark/src/KsTraceViewer.hpp
index cf529ba..15eee50 100644
--- a/kernel-shark/src/KsTraceViewer.hpp
+++ b/kernel-shark/src/KsTraceViewer.hpp
@@ -119,6 +119,8 @@ private:
 
 	KsDataStore		*_data;
 
+	QErrorMessage	_em;
+
 	enum Condition
 	{
 		Containes = 0,
-- 
2.19.1

Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Slavomir Kaslev]

On Thu, Apr 18, 2019 at 6:23 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> Markers can point to entries that are filtered out. When switching
> the marker it may happen that new Active Marker points to an entry
> that is filtered. In this case no actions must be taken and a warning
> message for the user must be displayed.
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/KsTraceViewer.cpp | 27 +++++++++++++++++----------
>  kernel-shark/src/KsTraceViewer.hpp |  2 ++
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
> index 04a38b8..85da64f 100644
> --- a/kernel-shark/src/KsTraceViewer.cpp
> +++ b/kernel-shark/src/KsTraceViewer.cpp
> @@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
>    _graphFollowsCheckBox(this),
>    _graphFollows(true),
>    _mState(nullptr),
> -  _data(nullptr)
> +  _data(nullptr),
> +  _em(this)
>  {
>         this->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Maximum);
>
> @@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
>                 QModelIndex index =
>                         _proxyModel.mapFromSource(_model.index(row, 0));
>
> -               /*
> -                * The row of the active marker will be colored according to
> -                * the assigned property of the current state of the Dual
> -                * marker. Auto-scrolling is temporarily disabled because we
> -                * do not want to scroll to the position of the marker yet.
> -                */
> -               _view.setAutoScroll(false);
> -               _view.selectRow(index.row());
> -               _view.setAutoScroll(true);
> +               if (index.isValid()) {
> +                       /*
> +                        * The row of the active marker will be colored according to
> +                        * the assigned property of the current state of the Dual
> +                        * marker. Auto-scrolling is temporarily disabled because we
> +                        * do not want to scroll to the position of the marker yet.
> +                        */
> +                       _view.setAutoScroll(false);
> +                       _view.selectRow(index.row());
> +                       _view.setAutoScroll(true);
> +               } else {
> +                       _view.clearSelection();
> +                       QString err("The marker's entry is filtered out.");
> +                       _em.showMessage(err);

When is this error message going to be cleared? I don't think it
changes atm unless a new error comes along.

-- Slavi

Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Yordan Karadzhov]

On 19.04.19 г. 10:29 ч., Slavomir Kaslev wrote:
> On Thu, Apr 18, 2019 at 6:23 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>>
>> Markers can point to entries that are filtered out. When switching
>> the marker it may happen that new Active Marker points to an entry
>> that is filtered. In this case no actions must be taken and a warning
>> message for the user must be displayed.
>>
>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>> ---
>>   kernel-shark/src/KsTraceViewer.cpp | 27 +++++++++++++++++----------
>>   kernel-shark/src/KsTraceViewer.hpp |  2 ++
>>   2 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
>> index 04a38b8..85da64f 100644
>> --- a/kernel-shark/src/KsTraceViewer.cpp
>> +++ b/kernel-shark/src/KsTraceViewer.cpp
>> @@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
>>     _graphFollowsCheckBox(this),
>>     _graphFollows(true),
>>     _mState(nullptr),
>> -  _data(nullptr)
>> +  _data(nullptr),
>> +  _em(this)
>>   {
>>          this->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Maximum);
>>
>> @@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
>>                  QModelIndex index =
>>                          _proxyModel.mapFromSource(_model.index(row, 0));
>>
>> -               /*
>> -                * The row of the active marker will be colored according to
>> -                * the assigned property of the current state of the Dual
>> -                * marker. Auto-scrolling is temporarily disabled because we
>> -                * do not want to scroll to the position of the marker yet.
>> -                */
>> -               _view.setAutoScroll(false);
>> -               _view.selectRow(index.row());
>> -               _view.setAutoScroll(true);
>> +               if (index.isValid()) {
>> +                       /*
>> +                        * The row of the active marker will be colored according to
>> +                        * the assigned property of the current state of the Dual
>> +                        * marker. Auto-scrolling is temporarily disabled because we
>> +                        * do not want to scroll to the position of the marker yet.
>> +                        */
>> +                       _view.setAutoScroll(false);
>> +                       _view.selectRow(index.row());
>> +                       _view.setAutoScroll(true);
>> +               } else {
>> +                       _view.clearSelection();
>> +                       QString err("The marker's entry is filtered out.");
>> +                       _em.showMessage(err);
> 
> When is this error message going to be cleared? I don't think it
> changes atm unless a new error comes along.

The message will be displayed in a small widget (QErrorMessage). This 
widget has an "OK" button and a "Show this message again" checkbox.

To clear the message you can either press "OK" or close the widget.
If you uncheck the "Show this message again" checkbox, no message will 
be shown next time when the error occurs.

Thanks!
Yordan

> 
> -- Slavi
> 

Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Slavomir Kaslev]

On Fri, 2019-04-19 at 07:46 +0000, Yordan Karadzhov wrote:
> 
> On 19.04.19 г. 10:29 ч., Slavomir Kaslev wrote:
> > On Thu, Apr 18, 2019 at 6:23 PM Yordan Karadzhov <
> > ykaradzhov@vmware.com> wrote:
> > > Markers can point to entries that are filtered out. When
> > > switching
> > > the marker it may happen that new Active Marker points to an
> > > entry
> > > that is filtered. In this case no actions must be taken and a
> > > warning
> > > message for the user must be displayed.
> > > 
> > > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> > > ---
> > >   kernel-shark/src/KsTraceViewer.cpp | 27 +++++++++++++++++----
> > > ------
> > >   kernel-shark/src/KsTraceViewer.hpp |  2 ++
> > >   2 files changed, 19 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-
> > > shark/src/KsTraceViewer.cpp
> > > index 04a38b8..85da64f 100644
> > > --- a/kernel-shark/src/KsTraceViewer.cpp
> > > +++ b/kernel-shark/src/KsTraceViewer.cpp
> > > @@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
> > >     _graphFollowsCheckBox(this),
> > >     _graphFollows(true),
> > >     _mState(nullptr),
> > > -  _data(nullptr)
> > > +  _data(nullptr),
> > > +  _em(this)
> > >   {
> > >          this->setSizePolicy(QSizePolicy::Preferred,
> > > QSizePolicy::Maximum);
> > > 
> > > @@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
> > >                  QModelIndex index =
> > >                          _proxyModel.mapFromSource(_model.index(r
> > > ow, 0));
> > > 
> > > -               /*
> > > -                * The row of the active marker will be colored
> > > according to
> > > -                * the assigned property of the current state of
> > > the Dual
> > > -                * marker. Auto-scrolling is temporarily disabled
> > > because we
> > > -                * do not want to scroll to the position of the
> > > marker yet.
> > > -                */
> > > -               _view.setAutoScroll(false);
> > > -               _view.selectRow(index.row());
> > > -               _view.setAutoScroll(true);
> > > +               if (index.isValid()) {
> > > +                       /*
> > > +                        * The row of the active marker will be
> > > colored according to
> > > +                        * the assigned property of the current
> > > state of the Dual
> > > +                        * marker. Auto-scrolling is temporarily
> > > disabled because we
> > > +                        * do not want to scroll to the position
> > > of the marker yet.
> > > +                        */
> > > +                       _view.setAutoScroll(false);
> > > +                       _view.selectRow(index.row());
> > > +                       _view.setAutoScroll(true);
> > > +               } else {
> > > +                       _view.clearSelection();
> > > +                       QString err("The marker's entry is
> > > filtered out.");
> > > +                       _em.showMessage(err);
> > 
> > When is this error message going to be cleared? I don't think it
> > changes atm unless a new error comes along.
> 
> The message will be displayed in a small widget (QErrorMessage).
> This 
> widget has an "OK" button and a "Show this message again" checkbox.
> 
> To clear the message you can either press "OK" or close the widget.
> If you uncheck the "Show this message again" checkbox, no message
> will 
> be shown next time when the error occurs.

That makes sense, in that case it lgtm

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

-- Slavi

Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Slavomir Kaslev]

On Fri, Apr 19, 2019 at 10:46 AM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
>
>
> On 19.04.19 г. 10:29 ч., Slavomir Kaslev wrote:
> > On Thu, Apr 18, 2019 at 6:23 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> >>
> >> Markers can point to entries that are filtered out. When switching
> >> the marker it may happen that new Active Marker points to an entry
> >> that is filtered. In this case no actions must be taken and a warning
> >> message for the user must be displayed.
> >>
> >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> >> ---
> >>   kernel-shark/src/KsTraceViewer.cpp | 27 +++++++++++++++++----------
> >>   kernel-shark/src/KsTraceViewer.hpp |  2 ++
> >>   2 files changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/kernel-shark/src/KsTraceViewer.cpp b/kernel-shark/src/KsTraceViewer.cpp
> >> index 04a38b8..85da64f 100644
> >> --- a/kernel-shark/src/KsTraceViewer.cpp
> >> +++ b/kernel-shark/src/KsTraceViewer.cpp
> >> @@ -61,7 +61,8 @@ KsTraceViewer::KsTraceViewer(QWidget *parent)
> >>     _graphFollowsCheckBox(this),
> >>     _graphFollows(true),
> >>     _mState(nullptr),
> >> -  _data(nullptr)
> >> +  _data(nullptr),
> >> +  _em(this)
> >>   {
> >>          this->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Maximum);
> >>
> >> @@ -493,15 +494,21 @@ void KsTraceViewer::markSwitch()
> >>                  QModelIndex index =
> >>                          _proxyModel.mapFromSource(_model.index(row, 0));
> >>
> >> -               /*
> >> -                * The row of the active marker will be colored according to
> >> -                * the assigned property of the current state of the Dual
> >> -                * marker. Auto-scrolling is temporarily disabled because we
> >> -                * do not want to scroll to the position of the marker yet.
> >> -                */
> >> -               _view.setAutoScroll(false);
> >> -               _view.selectRow(index.row());
> >> -               _view.setAutoScroll(true);
> >> +               if (index.isValid()) {
> >> +                       /*
> >> +                        * The row of the active marker will be colored according to
> >> +                        * the assigned property of the current state of the Dual
> >> +                        * marker. Auto-scrolling is temporarily disabled because we
> >> +                        * do not want to scroll to the position of the marker yet.
> >> +                        */
> >> +                       _view.setAutoScroll(false);
> >> +                       _view.selectRow(index.row());
> >> +                       _view.setAutoScroll(true);
> >> +               } else {
> >> +                       _view.clearSelection();
> >> +                       QString err("The marker's entry is filtered out.");
> >> +                       _em.showMessage(err);
> >
> > When is this error message going to be cleared? I don't think it
> > changes atm unless a new error comes along.
>
> The message will be displayed in a small widget (QErrorMessage). This
> widget has an "OK" button and a "Show this message again" checkbox.
>
> To clear the message you can either press "OK" or close the widget.
> If you uncheck the "Show this message again" checkbox, no message will
> be shown next time when the error occurs.

That makes sense. In that case, the patch lgtm

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

-- Slavi

Re: [PATCH v2 8/8] kernel-shark: Handle the case when the marker points to a filtered entry

[Steven Rostedt]

On Fri, 19 Apr 2019 07:46:10 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> > When is this error message going to be cleared? I don't think it
> > changes atm unless a new error comes along.  
> 
> The message will be displayed in a small widget (QErrorMessage). This 
> widget has an "OK" button and a "Show this message again" checkbox.
> 
> To clear the message you can either press "OK" or close the widget.
> If you uncheck the "Show this message again" checkbox, no message will 
> be shown next time when the error occurs.

Hmm, that dialog does appear a bit much. I wonder if we could just
change the color of the marker, or even make it into a dashed line (if
possible), or grayed out. Something immediate when a filter filters
out a marker to let the user know that it was filtered out.

That pop up would be something I would want to disable permanently in
my own use cases.

-- Steve