[PATCH 0/8]Fixes needed befor KS 1.0

[PATCH 0/8]Fixes needed befor KS 1.0

[Yordan Karadzhov (VMware)]

Bug fixes and optimizations for the Capture dialog of KernelShark.

Yordan Karadzhov (VMware) (8):
  kernel-shark: Don't free the string field of KS_CONFIG_STRING document
  kernel-shark: Add method for getting all selected events
  kernel-shark: Optimize the command line options used by the capture
    dialog
  kernel-shark: Don't try to open data file if recording failed
  kernel-shark: Remove a duplicate error message
  kernel-shark: remove the "make install" suggestion for capture errors
  kernel-shark: Disconnect "capture errors" before closing KernelShark
    GUI
  kernel-shark: Less scary error message from Capture dialog

 kernel-shark/src/KsCaptureDialog.cpp  | 36 +++++++++++-----------
 kernel-shark/src/KsCaptureDialog.hpp  |  2 ++
 kernel-shark/src/KsMainWindow.cpp     | 43 ++++++++-------------------
 kernel-shark/src/KsMainWindow.hpp     |  2 ++
 kernel-shark/src/KsWidgetsLib.cpp     | 41 +++++++++++++++++++++++++
 kernel-shark/src/KsWidgetsLib.hpp     |  2 ++
 kernel-shark/src/libkshark-configio.c |  1 -
 7 files changed, 79 insertions(+), 48 deletions(-)

-- 
2.20.1

[PATCH 1/8] kernel-shark: Don't free the string field of KS_CONFIG_STRING document

[Yordan Karadzhov (VMware)]

In kshark_config_doc_get() if the outputted document is of type
KS_CONFIG_STRING, do not try to free the old string before setting
the new one. Note that those strings are owned by the json object.

Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Fixes: 	b3b5b6504b1 ("kernel-shark-qt: Add I/O for configuration data.")
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/libkshark-configio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel-shark/src/libkshark-configio.c b/kernel-shark/src/libkshark-configio.c
index 9106522..cac27a7 100644
--- a/kernel-shark/src/libkshark-configio.c
+++ b/kernel-shark/src/libkshark-configio.c
@@ -310,7 +310,6 @@ bool kshark_config_doc_get(struct kshark_config_doc *conf,
 			if (!get_jval(conf, key, &tmp->conf_doc))
 				goto fail;
 
-			free(val->conf_doc);
 			val->conf_doc =
 				(char *) json_object_get_string(tmp->conf_doc);
 			free(tmp);
-- 
2.20.1

[PATCH 2/8] kernel-shark: Add method for getting all selected events

[Yordan Karadzhov (VMware)]

A new method is added to the class KsEventsCheckBoxWidget. It returns
a list of strings containing all selected events. If the whole system
is selected (the top level checkbox is checked), only the name of the
system is added to the list.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsWidgetsLib.cpp | 41 +++++++++++++++++++++++++++++++
 kernel-shark/src/KsWidgetsLib.hpp |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/kernel-shark/src/KsWidgetsLib.cpp b/kernel-shark/src/KsWidgetsLib.cpp
index ea02b5e..1c55c79 100644
--- a/kernel-shark/src/KsWidgetsLib.cpp
+++ b/kernel-shark/src/KsWidgetsLib.cpp
@@ -706,6 +706,47 @@ KsEventsCheckBoxWidget::KsEventsCheckBoxWidget(struct tep_handle *tep,
 	_adjustSize();
 }
 
+/**
+ * @brief Get a list of all checked events. If the whole system is selected
+ *	  (the top level checkbox is checked), only the name of the system is
+ *	  added to the list.
+ *
+ * @param option: If True, "-e" is added as prefix to each element of the list.
+ *
+ * @returns A list of checked events or systems.
+ */
+QStringList KsEventsCheckBoxWidget::getCheckedEvents(bool option)
+{
+	QTreeWidgetItem *sysItem, *evtItem;
+	QStringList list;
+	QString optStr;
+	int nSys, nEvts;
+
+	if (option)
+		optStr = "-e";
+
+	nSys = _tree.topLevelItemCount();
+	for(int t = 0; t < nSys; ++t) {
+		sysItem = _tree.topLevelItem(t);
+		if (sysItem->checkState(0) == Qt::Checked) {
+			list << optStr + sysItem->text(0);
+		} else {
+			nEvts = sysItem->childCount();
+			for (int c = 0; c < nEvts; ++c) {
+				evtItem = sysItem->child(c);
+				if (evtItem->checkState(0) == Qt::Checked) {
+					list << optStr +
+						sysItem->text(0) +
+						":" +
+						evtItem->text(0);
+				}
+			}
+		}
+	}
+
+	return list;
+}
+
 /** Remove a System from the Checkbox tree. */
 void KsEventsCheckBoxWidget::removeSystem(QString name) {
 	QTreeWidgetItem *item =
diff --git a/kernel-shark/src/KsWidgetsLib.hpp b/kernel-shark/src/KsWidgetsLib.hpp
index 6f22374..0d79250 100644
--- a/kernel-shark/src/KsWidgetsLib.hpp
+++ b/kernel-shark/src/KsWidgetsLib.hpp
@@ -333,6 +333,8 @@ struct KsEventsCheckBoxWidget : public KsCheckBoxTreeWidget
 	KsEventsCheckBoxWidget(struct tep_handle *pe,
 			       QWidget *parent = nullptr);
 
+	QStringList getCheckedEvents(bool option);
+
 	void removeSystem(QString name);
 };
 
-- 
2.20.1

[PATCH 3/8] kernel-shark: Optimize the command line options used by the capture dialog

[Yordan Karadzhov (VMware)]

Do not include all events belonging to a given system if the whole system
is selected. Add only the name of the system instead. Do not specify a
plugin if the selected plugin is "nop".

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsCaptureDialog.cpp | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index b107595..7da6c08 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -143,23 +143,14 @@ QStringList KsCaptureControl::getArgs()
 	QStringList argv;
 
 	argv << "record";
-	argv << "-p" << _pluginsComboBox.currentText();
 
-	if (_eventsWidget.all()) {
-		argv << "-e" << "all";
-	} else {
-		QVector<int> evtIds = _eventsWidget.getCheckedIds();
-		tep_event *event;
+	if (_pluginsComboBox.currentText() != "nop")
+		argv << "-p" << _pluginsComboBox.currentText();
 
-		for (auto const &id: evtIds) {
-			event = tep_find_event(_localTEP, id);
-			if (!event)
-				continue;
-
-			argv << "-e" + QString(event->system) +
-				":" + QString(event->name);
-		}
-	}
+	if (_eventsWidget.all())
+		argv << "-e" << "all";
+	else
+		argv << _eventsWidget.getCheckedEvents(true);
 
 	argv << "-o" << outputFileName();
 	argv << _commandLineEdit.text().split(" ");
-- 
2.20.1

[PATCH 4/8] kernel-shark: Don't try to open data file if recording failed

[Yordan Karadzhov (VMware)]

In the case when the recording process failed, the Capture dialog should
not send a signal to the KernelShark GUI to open a new tracing data file.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsCaptureDialog.cpp | 10 +++++++++-
 kernel-shark/src/KsCaptureDialog.hpp |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index 7da6c08..c877094 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -314,6 +314,7 @@ KsCaptureMonitor::KsCaptureMonitor(QWidget *parent)
 : QWidget(parent),
   _mergedChannels(false),
   _argsModified(false),
+  _captureStatus(false),
   _panel(this),
   _name("Output display", this),
   _space("max size ", this),
@@ -448,6 +449,9 @@ void KsCaptureMonitor::_captureFinished(int exit, QProcess::ExitStatus status)
 		_consolOutput.appendPlainText(errMessage);
 
 		QCoreApplication::processEvents();
+		_captureStatus = false;
+	} else {
+		_captureStatus = true;
 	}
 }
 
@@ -531,8 +535,12 @@ void KsCaptureDialog::_setChannelMode(int state)
 
 void KsCaptureDialog::_sendOpenReq(const QString &fileName)
 {
-	QLocalSocket *socket = new QLocalSocket(this);
+	QLocalSocket *socket;
+
+	if (!_captureMon._captureStatus)
+		return;
 
+	socket = new QLocalSocket(this);
 	socket->connectToServer("KSCapture", QIODevice::WriteOnly);
 	if (socket->waitForConnected()) {
 		QByteArray block;
diff --git a/kernel-shark/src/KsCaptureDialog.hpp b/kernel-shark/src/KsCaptureDialog.hpp
index 2265704..b168a2b 100644
--- a/kernel-shark/src/KsCaptureDialog.hpp
+++ b/kernel-shark/src/KsCaptureDialog.hpp
@@ -119,6 +119,8 @@ public:
 	 */
 	bool		_argsModified;
 
+	bool		_captureStatus;
+
 private:
 	QVBoxLayout	_layout;
 
-- 
2.20.1

[PATCH 5/8] kernel-shark: Remove a duplicate error message

[Yordan Karadzhov (VMware)]

If the capture process finishes with an error the _captureError() slot
will print an error message. We do not need to print identical message
in _captureFinished().

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsMainWindow.cpp | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 1e03632..752a17a 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -1143,35 +1143,9 @@ void KsMainWindow::_captureStarted()
 	_captureLocalServer.listen("KSCapture");
 }
 
-/**
- * If the authorization could not be obtained because the user dismissed
- * the authentication dialog (clicked Cancel), pkexec exits with a return
- * value of 126.
- */
-#define PKEXEC_DISMISS_RET	126
-
 void KsMainWindow::_captureFinished(int ret, QProcess::ExitStatus st)
 {
-	QProcess *capture = (QProcess *)sender();
-
 	_captureLocalServer.close();
-
-	if (ret == PKEXEC_DISMISS_RET) {
-		/*
-		 * Authorization could not be obtained because the user
-		 * dismissed the authentication dialog.
-		 */
-		return;
-	}
-
-	if (ret != 0 || st != QProcess::NormalExit) {
-		QString message = "Capture process failed:<br>";
-
-		message += capture->errorString();
-		message += "<br>Try doing:<br> sudo make install";
-
-		_error(message, "captureFinishedErr", false, false);
-	}
 }
 
 void KsMainWindow::_captureError(QProcess::ProcessError error)
-- 
2.20.1

[PATCH 6/8] kernel-shark: remove the "make install" suggestion for capture errors

[Yordan Karadzhov (VMware)]

The message is not appropriate, although it is most likely that the
error occurs because the user didn't install KernelShark and it runs
the executable from its build location.

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

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 752a17a..3f238db 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -1154,8 +1154,6 @@ void KsMainWindow::_captureError(QProcess::ProcessError error)
 	QString message = "Capture process failed:<br>";
 
 	message += capture->errorString();
-	message += "<br>Try doing:<br> sudo make install";
-
 	_error(message, "captureFinishedErr", false, false);
 }
 
-- 
2.20.1

[PATCH 7/8] kernel-shark: Disconnect "capture errors" before closing KernelShark GUI

[Yordan Karadzhov (VMware)]

This is done because we do not want to show error messages if the Capture
dialog is not closed.

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

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 3f238db..55518a3 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -155,6 +155,16 @@ KsMainWindow::~KsMainWindow()
 
 	_data.clear();
 
+	/*
+	 * Do not show error messages if the "capture" process is still
+	 * running (Capture dialog is not closed).
+	 */
+	if (_capture.state() != QProcess::NotRunning) {
+		disconnect(_captureErrorConnection);
+		_capture.close();
+		_capture.waitForFinished();
+	}
+
 	if (kshark_instance(&kshark_ctx))
 		kshark_free(kshark_ctx);
 }
@@ -1129,8 +1139,9 @@ void KsMainWindow::_initCapture()
 	connect(&_capture,	SIGNAL(finished(int, QProcess::ExitStatus)),
 		this,		SLOT(_captureFinished(int, QProcess::ExitStatus)));
 
-	connect(&_capture,	&QProcess::errorOccurred,
-		this,		&KsMainWindow::_captureError);
+	_captureErrorConnection =
+		connect(&_capture,	&QProcess::errorOccurred,
+			this,		&KsMainWindow::_captureError);
 
 	connect(&_captureLocalServer,	&QLocalServer::newConnection,
 		this,			&KsMainWindow::_readSocket);
diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
index 22700d9..62e66a0 100644
--- a/kernel-shark/src/KsMainWindow.hpp
+++ b/kernel-shark/src/KsMainWindow.hpp
@@ -159,6 +159,8 @@ private:
 
 	QSettings	_settings;
 
+	QMetaObject::Connection		_captureErrorConnection;
+
 	void _open();
 
 	void _restoreSession();
-- 
2.20.1

[PATCH 8/8] kernel-shark: Less scary error message from Capture dialog

[Yordan Karadzhov (VMware)]

Most likely the reason for this error to occur is that the KernelShark GUI
is not open (was closed).

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsCaptureDialog.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index c877094..acba330 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -554,6 +554,9 @@ void KsCaptureDialog::_sendOpenReq(const QString &fileName)
 		socket->flush();
 		socket->disconnectFromServer();
 	} else {
-		_captureMon.print(socket->errorString());
+		QString error(socket->errorString());
+
+		error += "\n(maybe KernelShark GUI is not open)";
+		_captureMon.print(error);
 	}
 }
-- 
2.20.1

Re: [PATCH 6/8] kernel-shark: remove the "make install" suggestion for capture errors

[Steven Rostedt]

On Tue,  9 Jul 2019 18:56:48 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The message is not appropriate, although it is most likely that the
> error occurs because the user didn't install KernelShark and it runs
> the executable from its build location.=

I don't believe it is most likely, as I hit this error several times and
that wasn't the solution.

But we need to have an error stating what was missing.

-- Steve

> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/KsMainWindow.cpp | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 752a17a..3f238db 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -1154,8 +1154,6 @@ void KsMainWindow::_captureError(QProcess::ProcessError error)
>  	QString message = "Capture process failed:<br>";
>  
>  	message += capture->errorString();
> -	message += "<br>Try doing:<br> sudo make install";
> -
>  	_error(message, "captureFinishedErr", false, false);
>  }
>