[PATCH v2 0/6] Modifications toward KS 1.0

[PATCH v2 0/6] Modifications toward KS 1.0

[Yordan Karadzhov]

Small modifications needed before releasing KernelShark 1.0.

This is the second version of this series of patches. It includes two new
patches (5/6 and 6/6). The commented-out code in patches 1/6 and 2/6 is
removed.

Yordan Karadzhov (6):
  kernel-shark-qt: Rearrange the "Filter" top menu
  kernel-shark-qt: Cosmetic modifications in KsQuickContextMenu
  kernel-shark-qt: Make the selection in the Table less touchy
  kernel-shark-qt: Do not auto-scrolling  when the marker switches
  kernel-shark-qt: Add the CPU filters to the filter clearing method
  kernel-shark-qt: Fix bug in plugin actions execution

 kernel-shark-qt/src/KsMainWindow.cpp       | 46 ++++++++++++----
 kernel-shark-qt/src/KsMainWindow.hpp       |  6 +--
 kernel-shark-qt/src/KsQuickContextMenu.cpp | 63 +++++++---------------
 kernel-shark-qt/src/KsQuickContextMenu.hpp |  4 +-
 kernel-shark-qt/src/KsTraceViewer.cpp      | 27 +++++++++-
 kernel-shark-qt/src/KsTraceViewer.hpp      |  2 +
 kernel-shark-qt/src/KsUtils.cpp            |  2 +
 kernel-shark-qt/src/libkshark.c            |  3 +-
 8 files changed, 93 insertions(+), 60 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] kernel-shark-qt: Rearrange the "Filter" top menu

[Yordan Karadzhov]

Now the "Filter" top menu contains only "Show Task" and "Show CPU"
menu actions ("Hide" versions are removed).

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

diff --git a/kernel-shark-qt/src/KsMainWindow.cpp b/kernel-shark-qt/src/KsMainWindow.cpp
index a375126..8bb51a3 100644
--- a/kernel-shark-qt/src/KsMainWindow.cpp
+++ b/kernel-shark-qt/src/KsMainWindow.cpp
@@ -53,8 +53,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
   _listFilterSyncCBox(nullptr),
   _showEventsAction("Show events", this),
   _showTasksAction("Show tasks", this),
-  _hideTasksAction("Hide tasks", this),
-  _hideCPUsAction("Hide CPUs", this),
+  _showCPUsAction("Show CPUs", this),
   _advanceFilterAction("Advance Filtering", this),
   _clearAllFilters("Clear all filters", this),
   _cpuSelectAction("CPUs", this),
@@ -205,11 +204,8 @@ void KsMainWindow::_createActions()
 	connect(&_showTasksAction,	&QAction::triggered,
 		this,			&KsMainWindow::_showTasks);
 
-	connect(&_hideTasksAction,	&QAction::triggered,
-		this,			&KsMainWindow::_hideTasks);
-
-	connect(&_hideCPUsAction,	&QAction::triggered,
-		this,			&KsMainWindow::_hideCPUs);
+	connect(&_showCPUsAction,	&QAction::triggered,
+		this,			&KsMainWindow::_showCPUs);
 
 	connect(&_advanceFilterAction,	&QAction::triggered,
 		this,			&KsMainWindow::_advancedFiltering);
@@ -322,8 +318,7 @@ void KsMainWindow::_createMenus()
 
 	filter->addAction(&_showEventsAction);
 	filter->addAction(&_showTasksAction);
-	filter->addAction(&_hideTasksAction);
-	filter->addAction(&_hideCPUsAction);
+	filter->addAction(&_showCPUsAction);
 	filter->addAction(&_advanceFilterAction);
 	filter->addAction(&_clearAllFilters);
 
@@ -621,6 +616,39 @@ void KsMainWindow::_hideTasks()
 	dialog->show();
 }
 
+void KsMainWindow::_showCPUs()
+{
+	kshark_context *kshark_ctx(nullptr);
+	KsCheckBoxWidget *cpu_cbd;
+	KsCheckBoxDialog *dialog;
+
+	if (!kshark_instance(&kshark_ctx))
+		return;
+
+	cpu_cbd = new KsCPUCheckBoxWidget(_data.tep(), this);
+	dialog = new KsCheckBoxDialog(cpu_cbd, this);
+
+	if (!kshark_ctx->show_cpu_filter ||
+	    !kshark_ctx->show_cpu_filter->count) {
+		cpu_cbd->setDefault(true);
+	} else {
+		int nCPUs = tep_get_cpus(_data.tep());
+		QVector<bool> v(nCPUs, false);
+
+		for (int i = 0; i < nCPUs; ++i) {
+			if (tracecmd_filter_id_find(kshark_ctx->show_cpu_filter, i))
+				v[i] = true;
+		}
+
+		cpu_cbd->set(v);
+	}
+
+	connect(dialog,		&KsCheckBoxDialog::apply,
+		&_data,		&KsDataStore::applyPosCPUFilter);
+
+	dialog->show();
+}
+
 void KsMainWindow::_hideCPUs()
 {
 	kshark_context *kshark_ctx(nullptr);
diff --git a/kernel-shark-qt/src/KsMainWindow.hpp b/kernel-shark-qt/src/KsMainWindow.hpp
index 301acc9..44f7dd7 100644
--- a/kernel-shark-qt/src/KsMainWindow.hpp
+++ b/kernel-shark-qt/src/KsMainWindow.hpp
@@ -118,9 +118,7 @@ private:
 
 	QAction		_showTasksAction;
 
-	QAction		_hideTasksAction;
-
-	QAction		_hideCPUsAction;
+	QAction		_showCPUsAction;
 
 	QAction		_advanceFilterAction;
 
@@ -173,6 +171,8 @@ private:
 
 	void _hideTasks();
 
+	void _showCPUs();
+
 	void _hideCPUs();
 
 	void _advancedFiltering();
-- 
2.17.1

[PATCH v2 2/6] kernel-shark-qt: Cosmetic modifications in KsQuickContextMenu

[Yordan Karadzhov]

The following minor modifications in KsQuickContextMenu are introduced:

1. Removes the "Apply to list/graph" check-boxes.
2. Swaps the position of the Show / Hide actions
3. Adds "clear all filters" action.

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

diff --git a/kernel-shark-qt/src/KsQuickContextMenu.cpp b/kernel-shark-qt/src/KsQuickContextMenu.cpp
index 6c9c9ef..728ecbd 100644
--- a/kernel-shark-qt/src/KsQuickContextMenu.cpp
+++ b/kernel-shark-qt/src/KsQuickContextMenu.cpp
@@ -62,7 +62,7 @@ KsQuickContextMenu::KsQuickContextMenu(KsDataStore *data, size_t row,
   _addTaskPlotAction(this),
   _removeCPUPlotAction(this),
   _removeTaskPlotAction(this),
-  _deselectAction(this)
+  _clearAllFilters(this)
 {
 	typedef void (KsQuickContextMenu::*mfp)();
 	QString taskName, parentName, descr;
@@ -87,37 +87,14 @@ KsQuickContextMenu::KsQuickContextMenu(KsDataStore *data, size_t row,
 
 	parentName = parent->metaObject()->className();
 
-	addSection("Pointer menu");
+	addSection("Pointer filter menu");
 
-	if (parentName == "KsTraceViewer") {
-		_graphSyncCBox =
-			KsUtils::addCheckBoxToMenu(this, "Apply filters to Graph");
-
-		connect(_graphSyncCBox,	&QCheckBox::stateChanged,
-					&KsUtils::graphFilterSync);
-
-		/*
-		 * By defauls the filters will be append to the List (Table)
-		 * only.
-		 */
-		KsUtils::listFilterSync(true);
-		KsUtils::graphFilterSync(false);
-		_graphSyncCBox->setChecked(false);
-	}
-
-	if (parentName == "KsTraceGraph" &&
-	    (graphs = dynamic_cast<KsTraceGraph *>(parent))) {
-		_listSyncCBox =
-			KsUtils::addCheckBoxToMenu(this, "Apply filters to List");
-
-		connect(_listSyncCBox,	&QCheckBox::stateChanged,
-					&KsUtils::listFilterSync);
-
-		/* By defauls the filters will be append to the Graph only. */
-		KsUtils::graphFilterSync(true);
-		KsUtils::listFilterSync(false);
-		_listSyncCBox->setChecked(false);
-	}
+	descr = "Show task [";
+	descr += taskName;
+	descr += "-";
+	descr += QString("%1").arg(pid);
+	descr += "] only";
+	lamAddAction(&_showTaskAction, &KsQuickContextMenu::_showTask);
 
 	descr = "Hide task [";
 	descr += taskName;
@@ -126,30 +103,30 @@ KsQuickContextMenu::KsQuickContextMenu(KsDataStore *data, size_t row,
 	descr += "]";
 	lamAddAction(&_hideTaskAction, &KsQuickContextMenu::_hideTask);
 
-	descr = "Show task [";
-	descr += taskName;
-	descr += "-";
-	descr += QString("%1").arg(pid);
+	descr = "Show event [";
+	descr += kshark_get_event_name_easy(_data->rows()[_row]);
 	descr += "] only";
-	lamAddAction(&_showTaskAction, &KsQuickContextMenu::_showTask);
+	lamAddAction(&_showEventAction, &KsQuickContextMenu::_showEvent);
 
 	descr = "Hide event [";
 	descr += kshark_get_event_name_easy(_data->rows()[_row]);
 	descr += "]";
 	lamAddAction(&_hideEventAction, &KsQuickContextMenu::_hideEvent);
 
-	descr = "Show event [";
-	descr += kshark_get_event_name_easy(_data->rows()[_row]);
-	descr += "] only";
-	lamAddAction(&_showEventAction, &KsQuickContextMenu::_showEvent);
+	if (parentName == "KsTraceViewer") {
+		descr = QString("Show CPU [%1] only").arg(cpu);
+		lamAddAction(&_showCPUAction, &KsQuickContextMenu::_showCPU);
+	}
 
 	descr = QString("Hide CPU [%1]").arg(_data->rows()[_row]->cpu);
 	lamAddAction(&_hideCPUAction, &KsQuickContextMenu::_hideCPU);
 
-	if (parentName == "KsTraceViewer") {
-		descr = QString("Show CPU [%1] only").arg(cpu);
-		lamAddAction(&_showCPUAction, &KsQuickContextMenu::_showCPU);
+	descr = "Clear all filters";
+	lamAddAction(&_clearAllFilters, &KsQuickContextMenu::_clearFilters);
 
+	addSection("Pointer plot menu");
+
+	if (parentName == "KsTraceViewer") {
 		descr = "Add [";
 		descr += taskName;
 		descr += "-";
diff --git a/kernel-shark-qt/src/KsQuickContextMenu.hpp b/kernel-shark-qt/src/KsQuickContextMenu.hpp
index f5a2a78..df8a65b 100644
--- a/kernel-shark-qt/src/KsQuickContextMenu.hpp
+++ b/kernel-shark-qt/src/KsQuickContextMenu.hpp
@@ -85,6 +85,8 @@ private:
 
 	QVector<int> _getFilterVector(tracecmd_filter_id *filter, int newId);
 
+	void _clearFilters() {_data->clearAllFilters();}
+
 	KsDataStore	*_data;
 
 	size_t		_row;
@@ -105,7 +107,7 @@ private:
 
 	QAction _removeTaskPlotAction;
 
-	QAction _deselectAction;
+	QAction _clearAllFilters;
 };
 
 /**
-- 
2.17.1

Re: [PATCH v2 2/6] kernel-shark-qt: Cosmetic modifications in KsQuickContextMenu

[Steven Rostedt]

On Wed,  9 Jan 2019 15:09:41 +0200
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> The following minor modifications in KsQuickContextMenu are introduced:
> 
> 1. Removes the "Apply to list/graph" check-boxes.
> 2. Swaps the position of the Show / Hide actions
> 3. Adds "clear all filters" action.
> 
This should actually be three separate patches. Even if they are minor,
each patch should only do one thing, with the exception of formatting
clean ups. Formatting clean ups can be done when the code that's being
cleaned up is being changed.

But the above are three distinct changes, and should be three distinct
patches. I know it's a little tedious to do it this way, but it's the
cleaner approach.

Can you resend with them separate?

-- Steve


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

Re: [PATCH v2 2/6] kernel-shark-qt: Cosmetic modifications in KsQuickContextMenu

[Yordan Karadzhov]

On 9.01.19 г. 18:33 ч., Steven Rostedt wrote:
> On Wed,  9 Jan 2019 15:09:41 +0200
> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> 
>> The following minor modifications in KsQuickContextMenu are introduced:
>>
>> 1. Removes the "Apply to list/graph" check-boxes.
>> 2. Swaps the position of the Show / Hide actions
>> 3. Adds "clear all filters" action.
>>
> This should actually be three separate patches. Even if they are minor,
> each patch should only do one thing, with the exception of formatting
> clean ups. Formatting clean ups can be done when the code that's being
> cleaned up is being changed.
> 
> But the above are three distinct changes, and should be three distinct
> patches. I know it's a little tedious to do it this way, but it's the
> cleaner approach.
> 
> Can you resend with them separate?
> 

Yes, I can do this.
Is there anything else that has to change in this series?

Thanks!
Yordan

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

Re: [PATCH v2 2/6] kernel-shark-qt: Cosmetic modifications in KsQuickContextMenu

[Steven Rostedt]

On Wed, 9 Jan 2019 16:38:09 +0000
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> > Can you resend with them separate?
> >   
> 
> Yes, I can do this.
> Is there anything else that has to change in this series?

So far no, I'm still looking at it.

You can send the three patches (split from this one) as a separate
series, and not wait for this series to be finished for review.

Thanks!

(and also I didn't have to play with these patches before applying
them ;-)

-- Steve

[PATCH v2 3/6] kernel-shark-qt: Make the selection in the Table less touchy

[Yordan Karadzhov]

This patch aims to make the selection in the table by using the
mouse more intuitive (less touchy). First of all, it disables
the auto-scrolling in horizontal direction. In addition to this,
it makes sure that all columns of the table have proper sizes
when the main window gets resized by the user.

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

diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index d64c2af..2418de3 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -30,6 +30,23 @@ void KsTableView::mousePressEvent(QMouseEvent *e) {
 	QTableView::mousePressEvent(e);
 }
 
+/**
+ * Reimplemented the handler for Auto-scrolling. With this we disable
+ * the Horizontal Auto-scrolling.
+ */
+void KsTableView::scrollTo(const QModelIndex &index, ScrollHint hint)
+{
+	int bottomMargin(2);
+
+	if (hint == QAbstractItemView::EnsureVisible &&
+	    index.row() > indexAt(rect().topLeft()).row() &&
+	    index.row() < indexAt(rect().bottomLeft()).row() - bottomMargin)
+		return;
+
+	QTableView::scrollTo(index, hint);
+}
+
+
 /** Create a default (empty) Trace viewer widget. */
 KsTraceViewer::KsTraceViewer(QWidget *parent)
 : QWidget(parent),
@@ -588,6 +605,7 @@ void KsTraceViewer::resizeEvent(QResizeEvent* event)
 	int nColumns = _tableHeader.count();
 	int tableSize(0), viewSize, freeSpace;
 
+	_resizeToContents();
 	for (int c = 0; c < nColumns; ++c) {
 		tableSize += _view.columnWidth(c);
 	}
diff --git a/kernel-shark-qt/src/KsTraceViewer.hpp b/kernel-shark-qt/src/KsTraceViewer.hpp
index a89fce1..a8c1fe6 100644
--- a/kernel-shark-qt/src/KsTraceViewer.hpp
+++ b/kernel-shark-qt/src/KsTraceViewer.hpp
@@ -33,6 +33,8 @@ public:
 	: QTableView(parent) {};
 
 	void mousePressEvent(QMouseEvent *event) override;
+
+	void scrollTo(const QModelIndex &index, ScrollHint hint) override;
 };
 
 /**
-- 
2.17.1

[PATCH v2 4/6] kernel-shark-qt: Do not auto-scrolling when the marker switches

[Yordan Karadzhov]

In some cases, the auto-scrolling (inside the table) to the
position of the marker, when the user switches between MarkerA
and MarkerB can be very annoying. This patch disables this
vertical auto-scrolling. Jumping to the position of the Active
marker is still possible, but the user has to do a second
click on the MarkerA/MarkerB button.

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

diff --git a/kernel-shark-qt/src/KsTraceViewer.cpp b/kernel-shark-qt/src/KsTraceViewer.cpp
index 2418de3..f02fbbb 100644
--- a/kernel-shark-qt/src/KsTraceViewer.cpp
+++ b/kernel-shark-qt/src/KsTraceViewer.cpp
@@ -578,13 +578,18 @@ void KsTraceViewer::markSwitch()
 		 */
 		size_t row =_mState->getMarker(state)._pos;
 
-		QModelIndex index = _proxyModel.mapFromSource(_model.index(row, 0));
+		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.
+		 * 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();
 	}
-- 
2.17.1

[PATCH v2 5/6] kernel-shark-qt: Add the CPU filters to the filter clearing method

[Yordan Karadzhov]

When we introdused the CPU filters, we forgot to add these filters to
void KsDataStore::clearAllFilters().

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

diff --git a/kernel-shark-qt/src/KsUtils.cpp b/kernel-shark-qt/src/KsUtils.cpp
index 0298010..34b2e2d 100644
--- a/kernel-shark-qt/src/KsUtils.cpp
+++ b/kernel-shark-qt/src/KsUtils.cpp
@@ -375,6 +375,8 @@ void KsDataStore::clearAllFilters()
 	kshark_filter_clear(kshark_ctx, KS_HIDE_TASK_FILTER);
 	kshark_filter_clear(kshark_ctx, KS_SHOW_EVENT_FILTER);
 	kshark_filter_clear(kshark_ctx, KS_HIDE_EVENT_FILTER);
+	kshark_filter_clear(kshark_ctx, KS_SHOW_CPU_FILTER);
+	kshark_filter_clear(kshark_ctx, KS_HIDE_CPU_FILTER);
 
 	tep_filter_reset(kshark_ctx->advanced_event_filter);
 	kshark_clear_all_filters(kshark_ctx, _rows, _dataSize);
-- 
2.17.1

[PATCH v2 6/6] kernel-shark-qt: Fix bug in plugin actions execution

[Yordan Karadzhov]

Plugin-provided actions are executed when loading the data. These
actions can be used to modify the contain of the kshark_entries
generated by a given event type and we consider the case of having
more than one plugin-provided actions per event type. However, the code
that handles the case of multiple actions per-event has a bug. The "if"
was introduced with the idea that only the last per-event action will
modify the KS_PLUGIN_UNTOUCHED flag of the entry, but it misbehaves in
the case of a single per-event action in the list, followed by actions
for other events.

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

diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
index 598ea52..9ab2d57 100644
--- a/kernel-shark-qt/src/libkshark.c
+++ b/kernel-shark-qt/src/libkshark.c
@@ -750,8 +750,7 @@ static size_t get_records(struct kshark_context *kshark_ctx,
 										entry->event_id))) {
 					evt_handler->event_func(kshark_ctx, rec, entry);
 					evt_handler = evt_handler->next;
-					if (!evt_handler)
-						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
+					entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
 				}
 
 				pid = entry->pid;
-- 
2.17.1

Re: [PATCH v2 6/6] kernel-shark-qt: Fix bug in plugin actions execution

[Steven Rostedt]

On Wed,  9 Jan 2019 15:09:45 +0200
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> Plugin-provided actions are executed when loading the data. These
> actions can be used to modify the contain of the kshark_entries

"modify the contain of" ?

> generated by a given event type and we consider the case of having
> more than one plugin-provided actions per event type. However, the code

 "more than one plugin-provided actions per event type." also doesn't
 make sense.

> that handles the case of multiple actions per-event has a bug. The "if"
> was introduced with the idea that only the last per-event action will
> modify the KS_PLUGIN_UNTOUCHED flag of the entry, but it misbehaves in
> the case of a single per-event action in the list, followed by actions
> for other events.

Patch looks fine, but can you resend with a cleaner change log?

Thanks!

-- Steve

> 
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark-qt/src/libkshark.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> index 598ea52..9ab2d57 100644
> --- a/kernel-shark-qt/src/libkshark.c
> +++ b/kernel-shark-qt/src/libkshark.c
> @@ -750,8 +750,7 @@ static size_t get_records(struct kshark_context *kshark_ctx,
>  										entry->event_id))) {
>  					evt_handler->event_func(kshark_ctx, rec, entry);
>  					evt_handler = evt_handler->next;
> -					if (!evt_handler)
> -						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> +					entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
>  				}
>  
>  				pid = entry->pid;

Re: [PATCH v2 6/6] kernel-shark-qt: Fix bug in plugin actions execution

[Steven Rostedt]

Ping?

-- Steve


On Wed, 9 Jan 2019 11:52:53 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed,  9 Jan 2019 15:09:45 +0200
> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> 
> > Plugin-provided actions are executed when loading the data. These
> > actions can be used to modify the contain of the kshark_entries  
> 
> "modify the contain of" ?
> 
> > generated by a given event type and we consider the case of having
> > more than one plugin-provided actions per event type. However, the code  
> 
>  "more than one plugin-provided actions per event type." also doesn't
>  make sense.
> 
> > that handles the case of multiple actions per-event has a bug. The "if"
> > was introduced with the idea that only the last per-event action will
> > modify the KS_PLUGIN_UNTOUCHED flag of the entry, but it misbehaves in
> > the case of a single per-event action in the list, followed by actions
> > for other events.  
> 
> Patch looks fine, but can you resend with a cleaner change log?
> 
> Thanks!
> 
> -- Steve
> 
> > 
> > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> > ---
> >  kernel-shark-qt/src/libkshark.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> > index 598ea52..9ab2d57 100644
> > --- a/kernel-shark-qt/src/libkshark.c
> > +++ b/kernel-shark-qt/src/libkshark.c
> > @@ -750,8 +750,7 @@ static size_t get_records(struct kshark_context *kshark_ctx,
> >  										entry->event_id))) {
> >  					evt_handler->event_func(kshark_ctx, rec, entry);
> >  					evt_handler = evt_handler->next;
> > -					if (!evt_handler)
> > -						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> > +					entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> >  				}
> >  
> >  				pid = entry->pid;  
> 

Re: [PATCH v2 6/6] kernel-shark-qt: Fix bug in plugin actions execution

[Steven Rostedt]

On Thu, 14 Mar 2019 20:01:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Ping?

Actually this is marked as superseded in patchwork. Is it?

-- Steve

> 
> On Wed, 9 Jan 2019 11:52:53 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed,  9 Jan 2019 15:09:45 +0200
> > Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> >   
> > > Plugin-provided actions are executed when loading the data. These
> > > actions can be used to modify the contain of the kshark_entries    
> > 
> > "modify the contain of" ?
> >   
> > > generated by a given event type and we consider the case of having
> > > more than one plugin-provided actions per event type. However, the code    
> > 
> >  "more than one plugin-provided actions per event type." also doesn't
> >  make sense.
> >   
> > > that handles the case of multiple actions per-event has a bug. The "if"
> > > was introduced with the idea that only the last per-event action will
> > > modify the KS_PLUGIN_UNTOUCHED flag of the entry, but it misbehaves in
> > > the case of a single per-event action in the list, followed by actions
> > > for other events.    
> > 
> > Patch looks fine, but can you resend with a cleaner change log?
> > 
> > Thanks!
> > 
> > -- Steve
> >   
> > > 
> > > Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> > > ---
> > >  kernel-shark-qt/src/libkshark.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> > > index 598ea52..9ab2d57 100644
> > > --- a/kernel-shark-qt/src/libkshark.c
> > > +++ b/kernel-shark-qt/src/libkshark.c
> > > @@ -750,8 +750,7 @@ static size_t get_records(struct kshark_context *kshark_ctx,
> > >  										entry->event_id))) {
> > >  					evt_handler->event_func(kshark_ctx, rec, entry);
> > >  					evt_handler = evt_handler->next;
> > > -					if (!evt_handler)
> > > -						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> > > +					entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
> > >  				}
> > >  
> > >  				pid = entry->pid;    
> >   

Re: [PATCH v2 6/6] kernel-shark-qt: Fix bug in plugin actions execution

[Yordan Karadzhov]

On 15.03.19 г. 2:05 ч., Steven Rostedt wrote:
> On Thu, 14 Mar 2019 20:01:14 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> Ping?
> 
> Actually this is marked as superseded in patchwork. Is it?
> 

I think this one was been pushed

https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/?id=fbba6a95533db0d42e6e9a9241fda3616b8f03a0

Thanks!
Yordan


> -- Steve
> 
>>
>> On Wed, 9 Jan 2019 11:52:53 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>>> On Wed,  9 Jan 2019 15:09:45 +0200
>>> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>>>    
>>>> Plugin-provided actions are executed when loading the data. These
>>>> actions can be used to modify the contain of the kshark_entries
>>>
>>> "modify the contain of" ?
>>>    
>>>> generated by a given event type and we consider the case of having
>>>> more than one plugin-provided actions per event type. However, the code
>>>
>>>   "more than one plugin-provided actions per event type." also doesn't
>>>   make sense.
>>>    
>>>> that handles the case of multiple actions per-event has a bug. The "if"
>>>> was introduced with the idea that only the last per-event action will
>>>> modify the KS_PLUGIN_UNTOUCHED flag of the entry, but it misbehaves in
>>>> the case of a single per-event action in the list, followed by actions
>>>> for other events.
>>>
>>> Patch looks fine, but can you resend with a cleaner change log?
>>>
>>> Thanks!
>>>
>>> -- Steve
>>>    
>>>>
>>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>>>> ---
>>>>   kernel-shark-qt/src/libkshark.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
>>>> index 598ea52..9ab2d57 100644
>>>> --- a/kernel-shark-qt/src/libkshark.c
>>>> +++ b/kernel-shark-qt/src/libkshark.c
>>>> @@ -750,8 +750,7 @@ static size_t get_records(struct kshark_context *kshark_ctx,
>>>>   										entry->event_id))) {
>>>>   					evt_handler->event_func(kshark_ctx, rec, entry);
>>>>   					evt_handler = evt_handler->next;
>>>> -					if (!evt_handler)
>>>> -						entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
>>>> +					entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK;
>>>>   				}
>>>>   
>>>>   				pid = entry->pid;
>>>    
>