[PATCH v2 0/3] Support "shell quoting" in the Record dialog

[PATCH v2 0/3] Support "shell quoting" in the Record dialog

[Yordan Karadzhov (VMware)]

A patch set implementing the improvements suggested here:
https://bugzilla.kernel.org/show_bug.cgi?id=204679

v2 changes:
  - Handling of multiple quotations and multi-line command arguments in
    patch 1/3.
  - Patches 2/3 and 3/3 are new.
  
Yordan Karadzhov (VMware) (3):
  kernel-shark: Provide parsing for quotation marks in Record command
    line
  kernel-shark: give more space to the command field of the Record
    dialog
  kernel-shark: Add quotation marks parsing example/test

 kernel-shark/examples/CMakeLists.txt |  4 +++
 kernel-shark/examples/cmd_split.cpp  | 52 ++++++++++++++++++++++++++++
 kernel-shark/src/KsCaptureDialog.cpp | 37 +++++++++++++++++---
 kernel-shark/src/KsCaptureDialog.hpp | 18 +++++++++-
 kernel-shark/src/KsUtils.cpp         | 41 ++++++++++++++++++++++
 kernel-shark/src/KsUtils.hpp         |  2 ++
 6 files changed, 148 insertions(+), 6 deletions(-)
 create mode 100644 kernel-shark/examples/cmd_split.cpp

-- 
2.20.1

[PATCH v2 1/3] kernel-shark: Provide parsing for quotation marks in Record command line

[Yordan Karadzhov (VMware)]

Shell-like parsing of quotation marks in the content of the "Command"
field of the "Record" dialog will give more options to the users.
For example, now we can trace

 python -c 'print("hello world")'

The patch also adds support for multi-line command options.

Suggested-by: Stephen Brennan <stephen@brennan.io>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204679
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsCaptureDialog.cpp | 10 +++++--
 kernel-shark/src/KsUtils.cpp         | 41 ++++++++++++++++++++++++++++
 kernel-shark/src/KsUtils.hpp         |  2 ++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index dc1e9b2..7c2ef46 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -170,7 +170,8 @@ QStringList KsCaptureControl::getArgs()
 		argv << _eventsWidget.getCheckedEvents(true);
 
 	argv << "-o" << outputFileName();
-	argv << _commandLineEdit.text().split(" ");
+
+	argv << KsUtils::splitArguments(_commandLineEdit.text());
 
 	return argv;
 }
@@ -350,7 +351,10 @@ void KsCaptureControl::_browse()
 
 void KsCaptureControl::_apply()
 {
-	emit argsReady(getArgs().join(" "));
+	QStringList argv = getArgs();
+
+	if (argv.count())
+		emit argsReady(argv.join(" "));
 }
 
 /** @brief Create KsCaptureMonitor widget. */
@@ -547,7 +551,7 @@ void KsCaptureDialog::_capture()
 	int argc;
 
 	if(_captureMon._argsModified) {
-		argv = _captureMon.text().split(" ");
+		argv = KsUtils::splitArguments(_captureMon.text());
 	} else {
 		argv = _captureCtrl.getArgs();
 	}
diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
index 58ce8c1..e99509f 100644
--- a/kernel-shark/src/KsUtils.cpp
+++ b/kernel-shark/src/KsUtils.cpp
@@ -257,6 +257,47 @@ QString getSaveFile(QWidget *parent,
 	return fileName;
 }
 
+/**
+ * Separate the command line arguments inside the string taking into account
+ * possible shell quoting and new lines.
+ */
+QStringList splitArguments(QString cmd)
+{
+	QString::SplitBehavior opt = QString::SkipEmptyParts;
+	int i, progress = 0, size;
+	QStringList argv;
+	QChar quote = 0;
+
+	/* Remove all new lines. */
+	cmd.replace("\\\n", " ");
+
+	size = cmd.count();
+	auto lamMid = [&] () {return cmd.mid(progress, i - progress);};
+	for (i = 0; i < size; ++i) {
+		if (cmd[i] == '\\') {
+			cmd.remove(i, 1);
+			size --;
+			continue;
+		}
+
+		if (cmd[i] == '\'' || cmd[i] == '"') {
+			if (quote.isNull()) {
+				argv << lamMid().split(" ", opt);
+				quote = cmd[i++];
+				progress = i;
+			} else if (quote == cmd[i]) {
+				argv << lamMid();
+				quote = 0;
+				progress = ++i;
+			}
+		}
+	}
+
+	argv << cmd.right(size - progress).split(" ", opt);
+
+	return argv;
+}
+
 }; // KsUtils
 
 /** A stream operator for converting QColor into KsPlot::Color. */
diff --git a/kernel-shark/src/KsUtils.hpp b/kernel-shark/src/KsUtils.hpp
index 7362c14..db1bf5e 100644
--- a/kernel-shark/src/KsUtils.hpp
+++ b/kernel-shark/src/KsUtils.hpp
@@ -130,6 +130,8 @@ QString getSaveFile(QWidget *parent,
 		    const QString &extension,
 		    QString &lastFilePath);
 
+QStringList splitArguments(QString cmd);
+
 }; // KsUtils
 
 /** Identifier of the Dual Marker active state. */
-- 
2.20.1

[PATCH v2 2/3] kernel-shark: give more space to the command field of the Record dialog

[Yordan Karadzhov (VMware)]

Since the Record dialog not support parsing of complex multi-line
commands the corresponding field of the widget has to be able to
accommodate such multi-line user input.

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

diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp
index 7c2ef46..fff42aa 100644
--- a/kernel-shark/src/KsCaptureDialog.cpp
+++ b/kernel-shark/src/KsCaptureDialog.cpp
@@ -29,6 +29,25 @@ static inline tep_handle *local_events()
 	return tracecmd_local_events(tracecmd_get_tracing_dir());
 }
 
+/**
+ * @brief Create KsCommandLineEdit.
+ *
+ * @param text: Defaulst text to be shown.
+ * @param parent: The parent of this widget.
+ */
+KsCommandLineEdit::KsCommandLineEdit(QString text, QWidget *parent)
+: QPlainTextEdit(text, parent) {}
+
+QSize KsCommandLineEdit::minimumSizeHint() const
+{
+	return {FONT_WIDTH * 30, FONT_HEIGHT * 2};
+}
+
+QSize KsCommandLineEdit::sizeHint() const
+{
+	return {FONT_WIDTH * 30, FONT_HEIGHT * 3};
+}
+
 /** @brief Create KsCaptureControl widget. */
 KsCaptureControl::KsCaptureControl(QWidget *parent)
 : QWidget(parent),
@@ -117,7 +136,11 @@ KsCaptureControl::KsCaptureControl(QWidget *parent)
 	_commandLabel.adjustSize();
 	_commandLabel.setFixedWidth(_outputLabel.width());
 	_execLayout.addWidget(&_commandLabel, row, 0);
+
 	_commandLineEdit.setFixedWidth(FONT_WIDTH * 30);
+	_commandLineEdit.setMinimumHeight(FONT_HEIGHT * 2);
+	_commandLineEdit.setMaximumHeight(FONT_HEIGHT * 9);
+
 	_execLayout.addWidget(&_commandLineEdit, row, 1);
 	_commandCheckBox.setCheckState(Qt::Unchecked);
 	_commandCheckBox.adjustSize();
@@ -171,7 +194,7 @@ QStringList KsCaptureControl::getArgs()
 
 	argv << "-o" << outputFileName();
 
-	argv << KsUtils::splitArguments(_commandLineEdit.text());
+	argv << KsUtils::splitArguments(_commandLineEdit.toPlainText());
 
 	return argv;
 }
@@ -282,7 +305,7 @@ void KsCaptureControl::_importSettings()
 		_outputLineEdit.setText(KS_C_STR_CAST(temp->conf_doc));
 
 	if (kshark_config_doc_get(conf, "Command", temp))
-		_commandLineEdit.setText(KS_C_STR_CAST(temp->conf_doc));
+		_commandLineEdit.setPlainText(KS_C_STR_CAST(temp->conf_doc));
 }
 
 void KsCaptureControl::_exportSettings()
@@ -330,7 +353,7 @@ void KsCaptureControl::_exportSettings()
 	kshark_config_doc_add(conf, "Output", kshark_json_to_conf(jout));
 
 	/* Save the command. */
-	comm = _commandLineEdit.text();
+	comm = _commandLineEdit.toPlainText();
 	json_object *jcomm = json_object_new_string(comm.toStdString().c_str());
 	kshark_config_doc_add(conf, "Command", kshark_json_to_conf(jcomm));
 
diff --git a/kernel-shark/src/KsCaptureDialog.hpp b/kernel-shark/src/KsCaptureDialog.hpp
index f8ddf4a..612080c 100644
--- a/kernel-shark/src/KsCaptureDialog.hpp
+++ b/kernel-shark/src/KsCaptureDialog.hpp
@@ -18,6 +18,20 @@
 // KernelShark
 #include "KsWidgetsLib.hpp"
 
+/**
+ * The KsCommandLineEdit class is used to override the default size hints of
+ * the QPlainTextEdit class.
+ */
+struct KsCommandLineEdit : public QPlainTextEdit
+{
+	KsCommandLineEdit(QString text, QWidget *parent = 0);
+
+private:
+	QSize sizeHint() const override;
+
+	QSize minimumSizeHint() const override;
+};
+
 /**
  * The KsCaptureControl class provides a control panel for the KernelShark
  * Capture dialog.
@@ -57,7 +71,9 @@ private:
 
 	QLabel		_pluginsLabel, _outputLabel, _commandLabel;
 
-	QLineEdit	_outputLineEdit, _commandLineEdit;
+	QLineEdit	_outputLineEdit;
+
+	KsCommandLineEdit	_commandLineEdit;
 
 	QToolBar	_settingsToolBar, _controlToolBar;
 
-- 
2.20.1

[PATCH v2 3/3] kernel-shark: Add quotation marks parsing example/test

[Yordan Karadzhov (VMware)]

The example implements a small GUI that executes shell commands.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/examples/CMakeLists.txt |  4 +++
 kernel-shark/examples/cmd_split.cpp  | 52 ++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 kernel-shark/examples/cmd_split.cpp

diff --git a/kernel-shark/examples/CMakeLists.txt b/kernel-shark/examples/CMakeLists.txt
index e16216e..35e6b1e 100644
--- a/kernel-shark/examples/CMakeLists.txt
+++ b/kernel-shark/examples/CMakeLists.txt
@@ -23,3 +23,7 @@ target_link_libraries(dplot   kshark-plot)
 message(STATUS "widgetdemo")
 add_executable(widgetdemo          widgetdemo.cpp)
 target_link_libraries(widgetdemo   kshark-gui)
+
+message(STATUS "cmd_split")
+add_executable(cmd_split           cmd_split.cpp)
+target_link_libraries(cmd_split    kshark-gui)
diff --git a/kernel-shark/examples/cmd_split.cpp b/kernel-shark/examples/cmd_split.cpp
new file mode 100644
index 0000000..ac68844
--- /dev/null
+++ b/kernel-shark/examples/cmd_split.cpp
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2017 VMware Inc, Yordan Karadzhov (VMware) <y.karadz@gmail.com>
+ */
+
+// C++
+#include<iostream>
+using namespace std;
+
+// Qt
+#include <QtWidgets>
+
+// KernelShark
+#include "KsUtils.hpp"
+
+int main(int argc, char **argv)
+{
+	QString text = "echo \"I want \\\" here\" \\\n \"and \\\' here\"";
+	QApplication a(argc, argv);
+	QStringList argList;
+	bool ok = true;
+	QProcess proc;
+
+	while (ok) {
+		text = QInputDialog::getMultiLineText(nullptr,
+						      "Shell quoting test",
+						      "Shell input:",
+						      text,
+						      &ok);
+
+		if (ok) {
+			argList = KsUtils::splitArguments(text);
+			qInfo() << argList;
+
+			proc.setProgram(argList.takeFirst());
+			proc.setArguments(argList);
+
+			proc.start();
+			proc.waitForFinished();
+
+			cout << proc.errorString().toStdString()
+			     << endl
+			     << proc.readAllStandardError().toStdString()
+			     << endl
+			     << proc.readAllStandardOutput().toStdString()
+			     << endl;
+		}
+	}
+
+	return 0;
+}
-- 
2.20.1

Re: [PATCH v2 3/3] kernel-shark: Add quotation marks parsing example/test

[Steven Rostedt]

On Wed, 18 Sep 2019 17:23:19 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The example implements a small GUI that executes shell commands.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>

The testing on this seems to do what I think it should do.

I did modify it with this change, to get rid of the "Unknown error"
message on success. Care to send a v3?

-- Steve

diff --git a/kernel-shark/examples/cmd_split.cpp b/kernel-shark/examples/cmd_split.cpp
index ac688442..b8cc1b59 100644
--- a/kernel-shark/examples/cmd_split.cpp
+++ b/kernel-shark/examples/cmd_split.cpp
@@ -39,9 +39,10 @@ int main(int argc, char **argv)
 			proc.start();
 			proc.waitForFinished();
 
-			cout << proc.errorString().toStdString()
-			     << endl
-			     << proc.readAllStandardError().toStdString()
+			if (proc.exitCode())
+				cout << proc.errorString().toStdString() << endl;
+
+			cout << proc.readAllStandardError().toStdString()
 			     << endl
 			     << proc.readAllStandardOutput().toStdString()
 			     << endl;

Re: [PATCH v2 1/3] kernel-shark: Provide parsing for quotation marks in Record command line

[Steven Rostedt]

On Wed, 18 Sep 2019 17:23:17 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> +/**
> + * Separate the command line arguments inside the string taking into account
> + * possible shell quoting and new lines.
> + */
> +QStringList splitArguments(QString cmd)
> +{
> +	QString::SplitBehavior opt = QString::SkipEmptyParts;
> +	int i, progress = 0, size;
> +	QStringList argv;
> +	QChar quote = 0;
> +
> +	/* Remove all new lines. */
> +	cmd.replace("\\\n", " ");
> +
> +	size = cmd.count();
> +	auto lamMid = [&] () {return cmd.mid(progress, i - progress);};
> +	for (i = 0; i < size; ++i) {
> +		if (cmd[i] == '\\') {
> +			cmd.remove(i, 1);
> +			size --;
> +			continue;
> +		}
> +
> +		if (cmd[i] == '\'' || cmd[i] == '"') {
> +			if (quote.isNull()) {
> +				argv << lamMid().split(" ", opt);
> +				quote = cmd[i++];
> +				progress = i;
> +			} else if (quote == cmd[i]) {
> +				argv << lamMid();
> +				quote = 0;
> +				progress = ++i;
> +			}
> +		}
> +	}
> +
> +	argv << cmd.right(size - progress).split(" ", opt);
> +
> +	return argv;
> +}

I still find the above hard to read, but so be it ;-) 

Anyway, not quite yet. I just noticed that if I do:

  echo "this \" is \" a \"test"

The output has:

("echo", "this \" is \" a \"test")


We don't want to keep the backslash here. We want to remove it before
passing it as an argument.

The above should be:

("echo", "this " is " a "test")

(thinking that you put in the outside quotes. In other words, if I have:

 echo "hello \"there\""

We should break that up into: 
  arg0=echo
  arg1=hello "there"

-- Steve

Re: [PATCH v2 1/3] kernel-shark: Provide parsing for quotation marks in Record command line

[Yordan Karadzhov (VMware)]

On 20.09.19 г. 2:20 ч., Steven Rostedt wrote:
> On Wed, 18 Sep 2019 17:23:17 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> +/**
>> + * Separate the command line arguments inside the string taking into account
>> + * possible shell quoting and new lines.
>> + */
>> +QStringList splitArguments(QString cmd)
>> +{
>> +	QString::SplitBehavior opt = QString::SkipEmptyParts;
>> +	int i, progress = 0, size;
>> +	QStringList argv;
>> +	QChar quote = 0;
>> +
>> +	/* Remove all new lines. */
>> +	cmd.replace("\\\n", " ");
>> +
>> +	size = cmd.count();
>> +	auto lamMid = [&] () {return cmd.mid(progress, i - progress);};
>> +	for (i = 0; i < size; ++i) {
>> +		if (cmd[i] == '\\') {
>> +			cmd.remove(i, 1);
>> +			size --;
>> +			continue;
>> +		}
>> +
>> +		if (cmd[i] == '\'' || cmd[i] == '"') {
>> +			if (quote.isNull()) {
>> +				argv << lamMid().split(" ", opt);
>> +				quote = cmd[i++];
>> +				progress = i;
>> +			} else if (quote == cmd[i]) {
>> +				argv << lamMid();
>> +				quote = 0;
>> +				progress = ++i;
>> +			}
>> +		}
>> +	}
>> +
>> +	argv << cmd.right(size - progress).split(" ", opt);
>> +
>> +	return argv;
>> +}
> 
> I still find the above hard to read, but so be it ;-)
> 
> Anyway, not quite yet. I just noticed that if I do:
> 
>    echo "this \" is \" a \"test"
> 
> The output has:
> 
> ("echo", "this \" is \" a \"test")
> 

Hi Steven,

This is a Qt printing artifact. The printout is trying to show clearly 
that this list contain only 2 strings. That is why it adds those 
backslashes. In the code of the example, try adding this:

                 if (ok) {
                         argList = KsUtils::splitArguments(text);
                         qInfo() << argList;
+                       for (auto a: argList)
+                               cout << a.toStdString() << endl;

                         proc.setProgram(argList.takeFirst());
                         proc.setArguments(argList);


and you will see what I mean.

Thanks a lot for being a tester!!!
Yordan

> 
> We don't want to keep the backslash here. We want to remove it before
> passing it as an argument.
> 
> The above should be:
> 
> ("echo", "this " is " a "test")
> 
> (thinking that you put in the outside quotes. In other words, if I have:
> 
>   echo "hello \"there\""
> 
> We should break that up into:
>    arg0=echo
>    arg1=hello "there"
> 
> -- Steve
> 

Re: [PATCH v2 3/3] kernel-shark: Add quotation marks parsing example/test

[Yordan Karadzhov (VMware)]

On 20.09.19 г. 2:12 ч., Steven Rostedt wrote:
> On Wed, 18 Sep 2019 17:23:19 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The example implements a small GUI that executes shell commands.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> 
> The testing on this seems to do what I think it should do.
> 
> I did modify it with this change, to get rid of the "Unknown error"
> message on success. Care to send a v3?

I don't care, just get it upstream.

Thanks!
Yordan

> 
> -- Steve
> 
> diff --git a/kernel-shark/examples/cmd_split.cpp b/kernel-shark/examples/cmd_split.cpp
> index ac688442..b8cc1b59 100644
> --- a/kernel-shark/examples/cmd_split.cpp
> +++ b/kernel-shark/examples/cmd_split.cpp
> @@ -39,9 +39,10 @@ int main(int argc, char **argv)
>   			proc.start();
>   			proc.waitForFinished();
>   
> -			cout << proc.errorString().toStdString()
> -			     << endl
> -			     << proc.readAllStandardError().toStdString()
> +			if (proc.exitCode())
> +				cout << proc.errorString().toStdString() << endl;
> +
> +			cout << proc.readAllStandardError().toStdString()
>   			     << endl
>   			     << proc.readAllStandardOutput().toStdString()
>   			     << endl;
>